Date   

[Patch] BaseTools: Remove CanSkip calling for incremental build

Bob Feng
 

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

If a module add a new PCD, the pcd token number will be
reassigned. The new Pcd token number should be updated
to all module's autogen files. CanSkip can only detect a
single module's change but not others. CanSkip block the
pcd token number update in incremental build, so this
patch is going to remove this call.

Signed-off-by: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/AutoGen/ModuleAutoGen.py | 3 ---
1 file changed, 3 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py b/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
index dc8b1fe3d1..eebf6e87f5 100755
--- a/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
@@ -1820,13 +1820,10 @@ class ModuleAutoGen(AutoGen):

if not self.IsLibrary and CreateLibraryCodeFile:
for LibraryAutoGen in self.LibraryAutoGenList:
LibraryAutoGen.CreateCodeFile()

- # CanSkip uses timestamps to determine build skipping
- if self.CanSkip():
- return
self.LibraryAutoGenList
AutoGenList = []
IgoredAutoGenList = []

for File in self.AutoGenFileList:
--
2.20.1.windows.1


[Patch] BaseTools: Clean the ffs folder before generating files in it

Bob Feng
 

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

The content in Guid.xref depends on the files under the corresponding
ffs folder.(refer to the commit 5e9256cd7f54ffd6f1fd9837df92a911fcd2d7c2)
To make Guid.xref update in the incremental build,
clean the files under that ffs folder before generating files in it.

Signed-off-by: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/GenFds/FfsFileStatement.py | 3 +++
1 file changed, 3 insertions(+)

diff --git a/BaseTools/Source/Python/GenFds/FfsFileStatement.py b/BaseTools/Source/Python/GenFds/FfsFileStatement.py
index 9fb62b0a91..1c6e59bac7 100644
--- a/BaseTools/Source/Python/GenFds/FfsFileStatement.py
+++ b/BaseTools/Source/Python/GenFds/FfsFileStatement.py
@@ -19,10 +19,11 @@ from Common.Misc import GuidStructureByteArrayToGuidString, SaveFileOnChange
import Common.LongFilePathOs as os
from .GuidSection import GuidSection
from .FvImageSection import FvImageSection
from .Ffs import FdfFvFileTypeToFileType
from .GenFdsGlobalVariable import GenFdsGlobalVariable
+import shutil

## generate FFS from FILE
#
#
class FileStatement (FileStatementClassObject):
@@ -65,10 +66,12 @@ class FileStatement (FileStatementClassObject):

Str = self.NameGuid
if FvName:
Str += FvName
OutputDir = os.path.join(GenFdsGlobalVariable.FfsDir, Str)
+ if os.path.exists(OutputDir):
+ shutil.rmtree(OutputDir)
if not os.path.exists(OutputDir):
os.makedirs(OutputDir)

if Dict is None:
Dict = {}
--
2.20.1.windows.1


[PATCH] BaseTools: Normalize case of pathname when evaluating Macros.

Bob Feng
 

From: Mingyue Liang <mingyuex.liang@intel.com>

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

Currently, When doing the Incremental build, the directory
macros extended to absolute path in output Makefile, which
is inconsistent with the output of Clean build.

When we do macro replacement, we can't replace macro due to
inconsistent path case, which results in inconsistent display
of incremental build and clean build in makefile.Therefore,
the path is converted to achieve the correct macro replacement.

Signed-off-by: Mingyue Liang <mingyuex.liang@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/AutoGen/GenMake.py | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py b/BaseTools/Source/Python/AutoGen/GenMake.py
index 0314d0ea34..b04d3f5436 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -786,8 +786,10 @@ cleanlib:

def ReplaceMacro(self, str):
for Macro in self.MacroList:
- if self._AutoGenObject.Macros[Macro] and self._AutoGenObject.Macros[Macro] in str:
- str = str.replace(self._AutoGenObject.Macros[Macro], '$(' + Macro + ')')
+ if self._AutoGenObject.Macros[Macro] and os.path.normcase(self._AutoGenObject.Macros[Macro]) in os.path.normcase(str):
+ replace_dir = str[os.path.normcase(str).index(os.path.normcase(self._AutoGenObject.Macros[Macro])): os.path.normcase(str).index(
+ os.path.normcase(self._AutoGenObject.Macros[Macro])) + len(self._AutoGenObject.Macros[Macro])]
+ str = str.replace(replace_dir, '$(' + Macro + ')')
return str

def CommandExceedLimit(self):
--
2.28.0.windows.1


[PATCH] BaseTools: Add included files to deps_target file.

Bob Feng
 

From: Mingyue Liang <mingyuex.liang@intel.com>

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

After changing the name of the include source file,
when doing incremental build, the previous source file
is not covered in the. DEPs file, and a build error occurs.

The root cause is that the build tools filter out some dependency
files, which are listed in inf source section, from the deps_target file.
Add those files back to deps_target file to resolve the above problem.

Signed-off-by: Mingyue Liang <mingyuex.liang@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/AutoGen/IncludesAutoGen.py | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
index 720d93395a..c3e6333217 100644
--- a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
@@ -103,7 +103,7 @@ ${END}
if os.path.normpath(dependency_file +".deps") == abspath:
continue
filename = os.path.basename(dependency_file).strip()
- if filename not in self.SourceFileList and filename not in targetname:
+ if filename not in targetname:
includes.add(dependency_file.strip())

for item in lines[1:]:
@@ -116,8 +116,6 @@ ${END}
if os.path.normpath(dependency_file +".deps") == abspath:
continue
filename = os.path.basename(dependency_file).strip()
- if filename in self.SourceFileList:
- continue
if filename in targetname:
continue
includes.add(dependency_file.strip())
--
2.28.0.windows.1


Re: [PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others

Laszlo Ersek
 

On 09/23/20 10:08, Malgorzata Kukiello wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2982

During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed
the way memory attributes are handled it affected a function in Page.c that
would clear all memory attributes even security related ones

Signed-off-by: Malgorzata Kukiello <jacek.kukiello@intel.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 05b82e0be1..2b1b72d862 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -113,7 +113,8 @@ typedef enum {
// Attributes bitmasks, grouped by type
//
#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
-#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
+#define EFI_MEMORY_ACCESS_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
+#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_ACCESS_MASK | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)

///
/// Memory descriptor version number.
(1) The subject line of this patch is wrong. The patch modifies MdePkg,
not MdeModulePkg. I suggest:

MdePkg/UefiSpec: separate page access bitmask from SP and CRYPTO caps

(2) My points (1) through (4) that I made under the other patch in this
series apply here as well, wrt. CC'ing maintainers / reviewers, commit
hash reference, structuring this posting etc. Note that this patch will
have a different set of required CC's.

(3) The commit message is lacking, in my opinion.

The point is that our workaround, in the UEFI memmap construction, must
not clear the SP and CRYPTO bits, because OSes do correctly interpret SP
and CRYPTO as capabilities. For this reason, the SP and CRYPTO bits
should be separated from the bitmask that we use for hiding the
page-access attributes.

Thanks
Laszlo


Re: [PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others

Laszlo Ersek
 

On 09/23/20 10:08, Malgorzata Kukiello wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2982

During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed
the way memory attributes are handled it affected a function in Page.c that
would clear all memory attributes even security related ones

Signed-off-by: Malgorzata Kukiello <jacek.kukiello@intel.com>
---
MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 2c2c9cd6c3..9254afb92b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1933,7 +1933,7 @@ CoreGetMemoryMap (
MemoryMapEnd = MemoryMap;
MemoryMap = MemoryMapStart;
while (MemoryMap < MemoryMapEnd) {
- MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
+ MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ACCESS_MASK;
MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
}
MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
(1) Please CC the maintainers that need to review this patch.

I'm doing that for you, for now.

$ python BaseTools/Scripts/GetMaintainer.py -l \
MdeModulePkg/Core/Dxe/Mem/Page.c

(But note that "BaseTools/Scripts/GetMaintainer.py" can operate on
patches / commits as well; it's better to use it that way, and then to
add corresponding "Cc:" tags to the commit messages themselves.)


(2) Also CC'ing Oleksiy (author of the commit in question), and Ard
(Linux EFI subsystem maintainer).


(3) Your commit reference in the commit message is *still* wrong (I
corrected it before in the BZ). The proper (upstream) commit hash is
3bd5c994c879f78e8e3d5346dc3b627f199291aa.


(4) This patch should be patch#2 in a series of two patches.

Currently this patch has been posted as a thread starter, and the more
foundational patch (the one that modifies the macros in "UefiSpec.h")
has been posted in response to this one.

Instead, there should be a cover letter 0/2, then 1/2 (in response to
the cover letter) should modify the macros, then finally this patch
should be 2/2.

Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone; it will
set up shallow threading (meaning sendemail.thread=True AND
sendemail.chainreplyto=False).


(5) A better subject line for this patch, in my opinion, would be:

MdeModulePkg/Core/Dxe: expose SP and CRYPTO capabilities in UEFI memmap

Because, near the location that you are modifying, we have the following
helpful comment:

//
// Note: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
// set attributes and change memory paging attribute accordingly.
// But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
// value from Capabilities in GCD memory map. This might cause
// boot problems. Clearing all paging related capabilities can
// workaround it. Following code is supposed to be removed once
// the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in
// UEFI spec and adopted by both EDK-II Core and all supported
// OSs.
//

Which in fact clarifies your issue report: apparently, the SP and CRYPTO
attributes *are* already treated as capabilities (and not as presently
set attributes) by operating systems. Therefore we should not hide these
capabilities from OSes -- we should not allow the workaround to cover
these capabilities.

(BTW, when Oleksiy posted the original patch and we reviewed it, I
believe this information -- i.e. how OSes would treat these new
capabilities -- was not at our disposal. So I think it was impossible to
tell whether we should hide SP and CRYPTO too, or not.)


(6) I think the comment I quoted above should be clarified too. It
currently says "Clearing all paging related capabilities". It should say
"Clearing all page-access permission capabilities", or something
similar; to better reflect the purpose of EFI_MEMORY_ACCESS_MASK.

Thanks
Laszlo


Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Ard Biesheuvel
 

On 9/23/20 12:04 PM, Laszlo Ersek wrote:
On 09/23/20 11:43, Laszlo Ersek wrote:
On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)
... on a second thought, we could merge this series in a single PR as
well; only edk2-platforms would have to advance its edk2 submodule
reference in two stages:
- first advance the submodule to patch#8,
- then update its own platform DSC files with the new lib instances,
- then advance the edk2 submodule again, to patch#14.
If that works for you, I think we should merge this edk2 set in one go
-- less work for me, and much more intuitive when viewed from the edk2
side. (The series would not be stuck in some half-merged state for any
time at all.)
We don't actually use git submodules there, so this does not work.

But I am fine with just merging this, as edk2-platforms has been reported to be in broken state anyway.


Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Laszlo Ersek
 

On 09/23/20 11:43, Laszlo Ersek wrote:
On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)
... on a second thought, we could merge this series in a single PR as
well; only edk2-platforms would have to advance its edk2 submodule
reference in two stages:

- first advance the submodule to patch#8,
- then update its own platform DSC files with the new lib instances,
- then advance the edk2 submodule again, to patch#14.

If that works for you, I think we should merge this edk2 set in one go
-- less work for me, and much more intuitive when viewed from the edk2
side. (The series would not be stuck in some half-merged state for any
time at all.)

Thanks
Laszlo


Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Laszlo Ersek
 

On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)

Thanks,
Laszlo


Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Ard Biesheuvel
 

On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?
(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most platforms there are broken atm anyway due to the RngLib change having been merged, but it would be good to have an idea what the status is there.


[PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others

Malgorzata Kukiello <jacek.kukiello@...>
 

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

During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed
the way memory attributes are handled it affected a function in Page.c that
would clear all memory attributes even security related ones

Signed-off-by: Malgorzata Kukiello <jacek.kukiello@intel.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 05b82e0be1..2b1b72d862 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -113,7 +113,8 @@ typedef enum {
// Attributes bitmasks, grouped by type
//
#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
-#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
+#define EFI_MEMORY_ACCESS_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
+#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_ACCESS_MASK | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)

///
/// Memory descriptor version number.
--
2.18.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.


[PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others

Malgorzata Kukiello <jacek.kukiello@...>
 

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

During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed
the way memory attributes are handled it affected a function in Page.c that
would clear all memory attributes even security related ones

Signed-off-by: Malgorzata Kukiello <jacek.kukiello@intel.com>
---
MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 2c2c9cd6c3..9254afb92b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1933,7 +1933,7 @@ CoreGetMemoryMap (
MemoryMapEnd = MemoryMap;
MemoryMap = MemoryMapStart;
while (MemoryMap < MemoryMapEnd) {
- MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
+ MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ACCESS_MASK;
MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
}
MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
--
2.18.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.


Re: [PATCH v7 06/14] EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform

Bret Barkelew <debtech@...>
 

Huzzah! My hero!

--
[ Insert obscure pop-culture reference here. ]

On Sep 22, 2020, at 7:47 PM, Ni, Ray <ray.ni@...> wrote:



Reviewed-by: Ray Ni ray.ni@...


Re: [PATCH] uefi-sct/SctPkg: Correct issue with memory protection enabled.

Laszlo Ersek
 

On 09/23/20 00:13, Jeff Brasen wrote:
Any comments on this change?
I suggest CC'ing the maintainers responsible for reviewing this change.
(I don't know who they are, unfortunately -- is there a Maintainers.txt
file in the uefi-sct tree?)

Thanks
Laszlo



Thanks,

Jeff

________________________________
From: Jeff Brasen <jbrasen@nvidia.com>
Sent: Friday, September 11, 2020 11:23 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jeff Brasen <jbrasen@nvidia.com>
Subject: [PATCH] uefi-sct/SctPkg: Correct issue with memory protection enabled.

On systems with memory protection enabled the modification of local
function initialization data results in permission issue. Make a copy of
data prior to modification.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
.../UnicodeCollationBBTestFunction.c | 38 ++++++++++---------
.../UnicodeCollation2BBTestFunction.c | 38 ++++++++++---------
2 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation/BlackBoxTest/UnicodeCollationBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation/BlackBoxTest/UnicodeCollationBBTestFunction.c
index 6fa11e6c..e0b4c1d9 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation/BlackBoxTest/UnicodeCollationBBTestFunction.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation/BlackBoxTest/UnicodeCollationBBTestFunction.c
@@ -25,7 +25,7 @@ Abstract:
--*/





-#include "SctLib.h"
+#include "SctLib.h"

#include "UnicodeCollationBBTestMain.h"





@@ -337,6 +337,7 @@ BBTestStrLwrFunctionAutoTest (
};



CHAR16 TestDataSav[MAX_SIZE_OF_STRING + 1];

+ CHAR16 TestDataRw[MAX_SIZE_OF_STRING + 1];







@@ -368,14 +369,15 @@ BBTestStrLwrFunctionAutoTest (
//

// Backup current test data

//

+ CopyUnicodeString (TestDataRw, TestData[Index]);

CopyUnicodeString (TestDataSav, TestData[Index]);



//

// For each test data, test the StrLwr functionality.

//

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);



- if (CheckStrLwr (TestDataSav, TestData[Index])) {

+ if (CheckStrLwr (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -390,15 +392,15 @@ BBTestStrLwrFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);





- CopyUnicodeString (TestDataSav, TestData[Index]);

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

+ CopyUnicodeString (TestDataSav, TestDataRw);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);



- if (CheckStrEql (TestDataSav, TestData[Index])) {

+ if (CheckStrEql (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -413,7 +415,7 @@ BBTestStrLwrFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);

};



@@ -458,6 +460,7 @@ BBTestStrUprFunctionAutoTest (
};



CHAR16 TestDataSav[MAX_SIZE_OF_STRING + 1];

+ CHAR16 TestDataRw[MAX_SIZE_OF_STRING + 1];







@@ -490,13 +493,14 @@ BBTestStrUprFunctionAutoTest (
// Backup current test data

//

CopyUnicodeString (TestDataSav, TestData[Index]);

+ CopyUnicodeString (TestDataRw, TestData[Index]);



//

// For each test data, test the StrUpr functionality.

//

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);



- if (CheckStrUpr (TestDataSav, TestData[Index])) {

+ if (CheckStrUpr (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -511,14 +515,14 @@ BBTestStrUprFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);



- CopyUnicodeString (TestDataSav, TestData[Index]);

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

+ CopyUnicodeString (TestDataSav, TestDataRw);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);



- if (CheckStrEql (TestDataSav, TestData[Index])) {

+ if (CheckStrEql (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -533,7 +537,7 @@ BBTestStrUprFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);

};



diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation2/BlackBoxTest/UnicodeCollation2BBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation2/BlackBoxTest/UnicodeCollation2BBTestFunction.c
index 653b263a..19ff6764 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation2/BlackBoxTest/UnicodeCollation2BBTestFunction.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/UnicodeCollation2/BlackBoxTest/UnicodeCollation2BBTestFunction.c
@@ -25,7 +25,7 @@ Abstract:
--*/





-#include "SctLib.h"
+#include "SctLib.h"

#include "UnicodeCollation2BBTestMain.h"



STATIC CONST STRICOLL_TEST_DATA_FIELD mStriCollTestData[] ={

@@ -335,6 +335,7 @@ BBTestStrLwrFunctionAutoTest (
};



CHAR16 TestDataSav[MAX_SIZE_OF_STRING + 1];

+ CHAR16 TestDataRw[MAX_SIZE_OF_STRING + 1];







@@ -367,13 +368,14 @@ BBTestStrLwrFunctionAutoTest (
// Backup current test data

//

CopyUnicodeString (TestDataSav, TestData[Index]);

+ CopyUnicodeString (TestDataRw, TestData[Index]);



//

// For each test data, test the StrLwr functionality.

//

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);



- if (CheckStrLwr (TestDataSav, TestData[Index])) {

+ if (CheckStrLwr (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -388,15 +390,15 @@ BBTestStrLwrFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);





- CopyUnicodeString (TestDataSav, TestData[Index]);

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

+ CopyUnicodeString (TestDataSav, TestDataRw);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);



- if (CheckStrEql (TestDataSav, TestData[Index])) {

+ if (CheckStrEql (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -411,7 +413,7 @@ BBTestStrLwrFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);

};



@@ -456,6 +458,7 @@ BBTestStrUprFunctionAutoTest (
};



CHAR16 TestDataSav[MAX_SIZE_OF_STRING + 1];

+ CHAR16 TestDataRw[MAX_SIZE_OF_STRING + 1];







@@ -488,13 +491,14 @@ BBTestStrUprFunctionAutoTest (
// Backup current test data

//

CopyUnicodeString (TestDataSav, TestData[Index]);

+ CopyUnicodeString (TestDataRw, TestData[Index]);



//

// For each test data, test the StrUpr functionality.

//

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);



- if (CheckStrUpr (TestDataSav, TestData[Index])) {

+ if (CheckStrUpr (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -509,14 +513,14 @@ BBTestStrUprFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);



- CopyUnicodeString (TestDataSav, TestData[Index]);

- UnicodeCollation->StrLwr (UnicodeCollation, TestData[Index]);

- UnicodeCollation->StrUpr (UnicodeCollation, TestData[Index]);

+ CopyUnicodeString (TestDataSav, TestDataRw);

+ UnicodeCollation->StrLwr (UnicodeCollation, TestDataRw);

+ UnicodeCollation->StrUpr (UnicodeCollation, TestDataRw);



- if (CheckStrEql (TestDataSav, TestData[Index])) {

+ if (CheckStrEql (TestDataSav, TestDataRw)) {

AssertionType = EFI_TEST_ASSERTION_PASSED;

} else {

AssertionType = EFI_TEST_ASSERTION_FAILED;

@@ -531,7 +535,7 @@ BBTestStrUprFunctionAutoTest (
__FILE__,

(UINTN)__LINE__,

TestDataSav,

- TestData[Index]

+ TestDataRw

);

};



--
2.25.1







Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Laszlo Ersek
 

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)

Thanks,
Laszlo


Re: [PATCH 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure

Laszlo Ersek
 

On 09/23/20 10:14, Laszlo Ersek wrote:

(3) Even better... can you modify GetApResetVectorSize() to take
&CpuMpData rather than &CpuMpData->AddressMap, and then check
CpuMpData->SevEsIsEnabled?

Hmmm, wait, that's not really simple, as we call GetApResetVectorSize()
from MpInitLibInitialize() too, way before we set
CpuMpData->SevEsIsEnabled from the PCD.

So I guess we should pass a dedicated BOOLEAN parameter to
GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in
MpInitLibInitialize(), we should pass in the PCD's value. At the call
site in AllocateResetVector(), we should pass in
CpuMpData->SevEsIsEnabled.

The reason I'm suggesting (3) is that I don't feel comfortable with
checking dynamic PCDs outside of entry point functions / initialization
functions.
You know what, never mind (3) -- I've just realized that
PcdCpuMaxLogicalProcessorNumber may be a dynamic PCD too. It might
require a lot of work to restrict all dynamic PCD accesses to the init
function only, and I couldn't necessarily justify all that work at the
moment (for myself or for anyone else).

So please consider (1), (2) and (4).

Thanks!
Laszlo


Re: [PATCH 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure

Laszlo Ersek
 

On 09/23/20 10:14, Laszlo Ersek wrote:
On 09/22/20 21:59, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

The AP reset vector stack allocation is only required if running as an
SEV-ES guest. Since the reset vector allocation is below 1MB in memory,
eliminate the requirement for bare-metal systems and non SEV-ES guests
to allocate the extra stack area, which can be large if the
PcdCpuMaxLogicalProcessorNumber value is large.

Cc: Garrett Kirkendall <garrett.kirkendall@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f639..39af2f9fba7d 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1152,7 +1152,15 @@ GetApResetStackSize (
VOID
)
{
- return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
+ //
+ // The AP reset stack is only used by SEV-ES guests. Don't add to the
+ // allocation if not necessary.
+ //
+ if (PcdGetBool (PcdSevEsIsEnabled) == TRUE) {
+ return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
+ } else {
+ return 0;
+ }
}

/**
Looks like this is a fix for commit 7b7508ad784d ("UefiCpuPkg: Allow AP
booting under SEV-ES", 2020-08-17).

In retrospect, that commit changed "ApResetVectorSize" -- which is
passed to GetWakeupBuffer() -- from value [a]

RendezvousFunnelSize + sizeof (MP_CPU_EXCHANGE_INFO)

to value [b]

ALIGN_VALUE ((RendezvousFunnelSize +
SwitchToRealSize +
sizeof (MP_CPU_EXCHANGE_INFO)),
CPU_STACK_ALIGNMENT) +
AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber)

The currently proposed patch does not entirely restore
"ApResetVectorSize" to the value it used to carry before commit
7b7508ad784d. Namely, while the patch eliminates the last addend, two
other changes from commit 7b7508ad784d remain in place:

- the addition of "SwitchToRealSize",
- the alignment up to CPU_STACK_ALIGNMENT.

As far as I understand, "SwitchToRealSize" is never zero
(AsmGetAddressMap() populates it with a difference of build-time
constants). I think it's not useful when SEV-ES is inactive.

Furthermore, the alignment for stack purposes is useless if we won't
have AP stacks (i.e., again when SEV-ES is inactive).

(1) Therefore I'd propose:

- folding GetApResetStackSize() into GetApResetVectorSize(),

- modifying GetApResetVectorSize() such that it return the original sum
[a] if SEV-ES is inactive, and the larger sum [b] if SEV-ES is active.

Hmmm. OK, maybe "SwitchToRealSize" should remain in place. It's a small
addition, and it reflects a code portion that is permanent. However, I
do think the alignment is both useless and confusing. If we won't
allocate an array of stacks, the alignment really makes no sense.


(2) A style comment: PcdGetBool()'s return value should not be compared
with TRUE or FALSE; just use PcdGetBool() as the whole controlling
expression for the "if".


(3) Even better... can you modify GetApResetVectorSize() to take
&CpuMpData rather than &CpuMpData->AddressMap, and then check
CpuMpData->SevEsIsEnabled?

Hmmm, wait, that's not really simple, as we call GetApResetVectorSize()
from MpInitLibInitialize() too, way before we set
CpuMpData->SevEsIsEnabled from the PCD.

So I guess we should pass a dedicated BOOLEAN parameter to
GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in
MpInitLibInitialize(), we should pass in the PCD's value. At the call
site in AllocateResetVector(), we should pass in
CpuMpData->SevEsIsEnabled.

The reason I'm suggesting (3) is that I don't feel comfortable with
checking dynamic PCDs outside of entry point functions / initialization
functions.
(4) The product

AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber)

includes the BSP. Is that intentional? Should we rather use:

AP_RESET_STACK_SIZE * (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)

?

(That would be a separate patch, of course.)

Thanks
Laszlo


Re: [PATCH 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure

Laszlo Ersek
 

On 09/22/20 21:59, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

The AP reset vector stack allocation is only required if running as an
SEV-ES guest. Since the reset vector allocation is below 1MB in memory,
eliminate the requirement for bare-metal systems and non SEV-ES guests
to allocate the extra stack area, which can be large if the
PcdCpuMaxLogicalProcessorNumber value is large.

Cc: Garrett Kirkendall <garrett.kirkendall@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f639..39af2f9fba7d 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1152,7 +1152,15 @@ GetApResetStackSize (
VOID
)
{
- return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
+ //
+ // The AP reset stack is only used by SEV-ES guests. Don't add to the
+ // allocation if not necessary.
+ //
+ if (PcdGetBool (PcdSevEsIsEnabled) == TRUE) {
+ return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
+ } else {
+ return 0;
+ }
}

/**
Looks like this is a fix for commit 7b7508ad784d ("UefiCpuPkg: Allow AP
booting under SEV-ES", 2020-08-17).

In retrospect, that commit changed "ApResetVectorSize" -- which is
passed to GetWakeupBuffer() -- from value [a]

RendezvousFunnelSize + sizeof (MP_CPU_EXCHANGE_INFO)

to value [b]

ALIGN_VALUE ((RendezvousFunnelSize +
SwitchToRealSize +
sizeof (MP_CPU_EXCHANGE_INFO)),
CPU_STACK_ALIGNMENT) +
AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber)

The currently proposed patch does not entirely restore
"ApResetVectorSize" to the value it used to carry before commit
7b7508ad784d. Namely, while the patch eliminates the last addend, two
other changes from commit 7b7508ad784d remain in place:

- the addition of "SwitchToRealSize",
- the alignment up to CPU_STACK_ALIGNMENT.

As far as I understand, "SwitchToRealSize" is never zero
(AsmGetAddressMap() populates it with a difference of build-time
constants). I think it's not useful when SEV-ES is inactive.

Furthermore, the alignment for stack purposes is useless if we won't
have AP stacks (i.e., again when SEV-ES is inactive).

(1) Therefore I'd propose:

- folding GetApResetStackSize() into GetApResetVectorSize(),

- modifying GetApResetVectorSize() such that it return the original sum
[a] if SEV-ES is inactive, and the larger sum [b] if SEV-ES is active.

Hmmm. OK, maybe "SwitchToRealSize" should remain in place. It's a small
addition, and it reflects a code portion that is permanent. However, I
do think the alignment is both useless and confusing. If we won't
allocate an array of stacks, the alignment really makes no sense.


(2) A style comment: PcdGetBool()'s return value should not be compared
with TRUE or FALSE; just use PcdGetBool() as the whole controlling
expression for the "if".


(3) Even better... can you modify GetApResetVectorSize() to take
&CpuMpData rather than &CpuMpData->AddressMap, and then check
CpuMpData->SevEsIsEnabled?

Hmmm, wait, that's not really simple, as we call GetApResetVectorSize()
from MpInitLibInitialize() too, way before we set
CpuMpData->SevEsIsEnabled from the PCD.

So I guess we should pass a dedicated BOOLEAN parameter to
GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in
MpInitLibInitialize(), we should pass in the PCD's value. At the call
site in AllocateResetVector(), we should pass in
CpuMpData->SevEsIsEnabled.

The reason I'm suggesting (3) is that I don't feel comfortable with
checking dynamic PCDs outside of entry point functions / initialization
functions.

Thanks,
Laszlo


Re: [PATCH v1 1/1] IntelSiliconPkg/ShadowMicrocodePei: Add PCD for shadowing all microcode.

Ni, Ray
 

MpInitLib already contains logic to shadow microcode to memory.
Is this still needed?

-----Original Message-----
From: Li, Aaron <aaron.li@intel.com>
Sent: Wednesday, August 12, 2020 3:55 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
<rangasai.v.chaganty@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
Subject: [PATCH v1 1/1] IntelSiliconPkg/ShadowMicrocodePei: Add PCD for
shadowing all microcode.

This patch is to add a PCD PcdShadowAllMicrocode to support shadowing
all microcode patch to memory.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2891

Signed-off-by: Aaron Li <aaron.li@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
---

Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
| 4 ++++

Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.i
nf | 3 +++
Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec | 7 +++++++
3 files changed, 14 insertions(+)

diff --git
a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.c
b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.c
index 8d6574f66794..5c7ee6910c8e 100644
---
a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.c
+++
b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.c
@@ -132,6 +132,10 @@ IsMicrocodePatchNeedLoad (
CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;

UINTN Index;



+ if (FeaturePcdGet (PcdShadowAllMicrocode)) {

+ return TRUE;

+ }

+

//

// Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch
header.

//

diff --git
a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.inf
b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.inf
index 019400ab31da..581780add891 100644
---
a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.inf
+++
b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei
.inf
@@ -39,5 +39,8 @@ [Guids]
gEdkiiMicrocodeShadowInfoHobGuid

gEdkiiMicrocodeStorageTypeFlashGuid



+[Pcd]

+ gIntelSiliconPkgTokenSpaceGuid.PcdShadowAllMicrocode

+

[Depex]

TRUE

diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
index e4a7fec3a3ea..3a12fe99fac6 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -76,6 +76,13 @@ [Protocols]
# Include/Protocol/PlatformDeviceSecurityPolicy.h

gEdkiiDeviceSecurityPolicyProtocolGuid = {0x7ea41a99, 0x5e32, 0x4c97,
{0x88, 0xc4, 0xd6, 0xe7, 0x46, 0x84, 0x9, 0xd4}}



+[PcdsFeatureFlag]

+ ## Indicates if all microcode update patches shall be shadowed to memory.

+ # TRUE - All microcode patches will be shadowed.<BR>

+ # FALSE - Only the microcode for current present processors will be
shadowed.<BR>

+ # @Prompt Shadow all microcode update patches.

+
gIntelSiliconPkgTokenSpaceGuid.PcdShadowAllMicrocode|FALSE|BOOLEAN|0x
00000006

+

[PcdsFixedAtBuild, PcdsPatchableInModule]

## Error code for VTd error.<BR><BR>

# EDKII_ERROR_CODE_VTD_ERROR = (EFI_IO_BUS_UNSPECIFIED |
(EFI_OEM_SPECIFIC | 0x00000000)) = 0x02008000<BR>

--
2.23.0.windows.1


Re: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

Bret Barkelew
 

To whom it may concern,

This is as done as it’s going to get.

 

Thank you all for your help!

 

- Bret

 

From: Bret Barkelew
Sent: Tuesday, September 22, 2020 11:08 PM
To: devel@edk2.groups.io
Cc: Yao, Jiewen; Dandan Bi; Chao Zhang; Jian J Wang; Hao A Wu; liming.gao; Jordan Justen; Laszlo Ersek; Ard Biesheuvel; Andrew Fish; Ni, Ray; Bret Barkelew
Subject: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

 

The 14 patches in this series add the VariablePolicy feature to the core,
deprecate Edk2VarLock (while adding a compatibility layer to reduce code
churn), and integrate the VariablePolicy libraries and protocols into
Variable Services.

Since the integration requires multiple changes, including adding libraries,
a protocol, an SMI communication handler, and VariableServices integration,
the patches are broken up by individual library additions and then a final
integration. Security-sensitive changes like bypassing Authenticated
Variable enforcement are also broken out into individual patches so that
attention can be called directly to them.

Platform porting instructions are described in this wiki entry:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-Protocol---Enhanced-Method-for-Managing-Variables%23platform-porting&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C1b508888579a496271ae08d85f870cf2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637364380959491498&amp;sdata=n2SYXG8iUBvDc2bo0%2B%2BB9Ftut46VNtVgdpJSFnX6%2Fu4%3D&amp;reserved=0

Discussion of the feature can be found in multiple places throughout
the last year on the RFC channel, staging branches, and in devel.

Most recently, this subject was discussed in this thread:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C1b508888579a496271ae08d85f870cf2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637364380959491498&amp;sdata=81ta2Tht%2FwjDjk8LqQ8wRI0I1ggK14fePhWlMxDOCGA%3D&amp;reserved=0
(the code branches shared in that discussion are now out of date, but the
whitepapers and discussion are relevant).

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <liming.gao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Andrew Fish <afish@...>
Cc: Ray Ni <ray.ni@...>
Cc: Bret Barkelew <brbarkel@...>
Signed-off-by: Bret Barkelew <brbarkel@...>

v8 changes:
* Rebase
* Small tweaks from final PRs
* Drank a lot
* Enrolled several members and a steward in CatFacts

v7 changes:
* Address comments from Dandan about security of the MM handler
* Add readme
* Fix bug around hex characters in BOOT####, etc
* Add additional testing for hex characters
* Add additional testing for authenticated variables

v6 changes:
* Fix an issue with uninitialized Status in InitVariablePolicyLib() and DeinitVariablePolicyLib()
* Fix GCC building in shell-based functional test
* Rebase on latest origin/master

v5 changes:
* Fix the CONST mismatch in VariablePolicy.h and VariablePolicySmmDxe.c
* Fix EFIAPI mismatches in the functional unittest
* Rebase on latest origin/master

v4 changes:
* Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD from platforms
* Rebase on master
* Migrate to new MmCommunicate2 protocol
* Fix an oversight in the default return value for InitMmCommonCommBuffer
* Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume variables

V3 changes:
* Address all non-unittest issues with ECC
* Make additional style changes
* Include section name in hunk headers in "ini-style" files
* Remove requirement for the EdkiiPiSmmCommunicationsRegionTable driver
  (now allocates its own buffer)
* Change names from VARIABLE_POLICY_PROTOCOL and gVariablePolicyProtocolGuid
  to EDKII_VARIABLE_POLICY_PROTOCOL and gEdkiiVariablePolicyProtocolGuid
* Fix GCC warning about initializing externs
* Add UNI strings for new PCD
* Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
* Reorder patches according to Liming's feedback about adding to platforms
  before changing variable driver

V2 changes:
* Fixed implementation for RuntimeDxe
* Add PCD to block DisableVariablePolicy
* Fix the DumpVariablePolicy pagination in SMM


Bret Barkelew (14):
  MdeModulePkg: Define the VariablePolicy protocol interface
  MdeModulePkg: Define the VariablePolicyLib
  MdeModulePkg: Define the VariablePolicyHelperLib
  MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
  OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
  EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
  ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
  UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg platform
  MdeModulePkg: Connect VariablePolicy business logic to
    VariableServices
  MdeModulePkg: Allow VariablePolicy state to delete protected variables
  SecurityPkg: Allow VariablePolicy state to delete authenticated
    variables
  MdeModulePkg: Change TCG MOR variables to use VariablePolicy
  MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
  MdeModulePkg: Add a shell-based functional test for VariablePolicy

 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c                               |  345 +++
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c                   |  396 ++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c                     |   46 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c               |   85 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c                               |  830 +++++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c   | 2452 ++++++++++++++++++++
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.c        | 2226 ++++++++++++++++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c                               |   52 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c                               |   60 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c                                    |   49 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c                                 |   56 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c                   |   71 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c                        |  642 +++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c                       |   14 +
 SecurityPkg/Library/AuthVariableLib/AuthService.c                                        |   30 +-
 ArmVirtPkg/ArmVirt.dsc.inc                                                               |    4 +
 EmulatorPkg/EmulatorPkg.dsc                                                              |    3 +
 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h                                            |   54 +
 MdeModulePkg/Include/Library/VariablePolicyHelperLib.h                                   |  164 ++
 MdeModulePkg/Include/Library/VariablePolicyLib.h                                         |  207 ++
 MdeModulePkg/Include/Protocol/VariablePolicy.h                                           |  157 ++
 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf                             |   42 +
 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni                             |   12 +
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf                 |   35 +
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni                 |   12 +
 MdeModulePkg/Library/VariablePolicyLib/ReadMe.md                                         |  410 ++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf                             |   49 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni                             |   12 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf                   |   51 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf |   45 +
 MdeModulePkg/MdeModulePkg.ci.yaml                                                        |    8 +-
 MdeModulePkg/MdeModulePkg.dec                                                            |   26 +-
 MdeModulePkg/MdeModulePkg.dsc                                                            |    9 +
 MdeModulePkg/MdeModulePkg.uni                                                            |    7 +
 MdeModulePkg/Test/MdeModulePkgHostTest.dsc                                               |   11 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/Readme.md                          |   55 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.inf      |   47 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyTestAuthVar.h        |  128 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf                        |    5 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf                               |    4 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf                     |   11 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf                      |    4 +
 OvmfPkg/OvmfPkgIa32.dsc                                                                  |    5 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                                               |    5 +
 OvmfPkg/OvmfPkgX64.dsc                                                                   |    5 +
 OvmfPkg/OvmfXen.dsc                                                                      |    4 +
 SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf                                  |    2 +
 UefiPayloadPkg/UefiPayloadPkgIa32.dsc                                                    |    4 +
 UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc                                                 |    4 +
 49 files changed, 8874 insertions(+), 81 deletions(-)
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
 create mode 100644 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
 create mode 100644 MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
 create mode 100644 MdeModulePkg/Include/Library/VariablePolicyLib.h
 create mode 100644 MdeModulePkg/Include/Protocol/VariablePolicy.h
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/Readme.md
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.inf
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyTestAuthVar.h

--
2.28.0.windows.1

 

16841 - 16860 of 82317