Re: [edk2] [Patch] BaseTool: Variable Merge.

[Gao, Liming]

Bob:
  Please update commit message title with the detail information. 

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>BobCF
>Sent: Friday, September 07, 2018 3:45 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: [edk2] [Patch] BaseTool: Variable Merge.
>
>If Structure PCD and Normal Pcd refer to the
>same variable, do variable merge.
>
>Enhance error message.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bob Feng 
>Cc: Liming Gao 
>---
> BaseTools/Source/Python/AutoGen/AutoGen.py |  3 +-
> BaseTools/Source/Python/AutoGen/GenVar.py  | 71
>--
> .../Source/Python/Workspace/BuildClassObject.py|  2 +
> BaseTools/Source/Python/Workspace/DscBuildData.py  |  3 +-
> 4 files changed, 60 insertions(+), 19 deletions(-)
>
>diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
>b/BaseTools/Source/Python/AutoGen/AutoGen.py
>index 95370d1821..d1bd1b1d4c 100644
>--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
>+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
>@@ -1211,11 +1211,11 @@ class PlatformAutoGen(AutoGen):
> continue
> if len(Sku.VariableName) > 0:
> VariableGuidStructure = Sku.VariableGuidValue
> VariableGuid =
>GuidStructureStringToGuidString(VariableGuidStructure)
> for StorageName in Sku.DefaultStoreDict:
>-VariableInfo.append_variable(var_info(Index, pcdname,
>StorageName, SkuName, StringToArray(Sku.VariableName), VariableGuid,
>Sku.VariableOffset, Sku.VariableAttribute, Sku.HiiDefaultValue,
>Sku.DefaultStoreDict[StorageName], Pcd.DatumType))
>+VariableInfo.append_variable(var_info(Index, pcdname,
>StorageName, SkuName, StringToArray(Sku.VariableName), VariableGuid,
>Sku.VariableOffset, Sku.VariableAttribute, Sku.HiiDefaultValue,
>Sku.DefaultStoreDict[StorageName], Pcd.DatumType,
>Pcd.CustomAttribute['DscPosition'], Pcd.CustomAttribute.get('IsStru',False)))
> Index += 1
> return VariableInfo
>
> def UpdateNVStoreMaxSize(self, OrgVpdFile):
> if self.VariableInfo:
>@@ -2104,10 +2104,11 @@ class PlatformAutoGen(AutoGen):
> EdkLogger.error('build', FORMAT_INVALID, Cause, 
> File=self.MetaFile,
> ExtraData="%s.%s" % 
> (ToPcd.TokenSpaceGuidCName,
>TokenCName))
> ToPcd.validateranges = FromPcd.validateranges
> ToPcd.validlists = FromPcd.validlists
> ToPcd.expressions = FromPcd.expressions
>+ToPcd.CustomAttribute = FromPcd.CustomAttribute
>
> if FromPcd is not None and ToPcd.DatumType == TAB_VOID and not
>ToPcd.MaxDatumSize:
> EdkLogger.debug(EdkLogger.DEBUG_9, "No MaxDatumSize specified
>for PCD %s.%s" \
> % (ToPcd.TokenSpaceGuidCName, TokenCName))
> Value = ToPcd.DefaultValue
>diff --git a/BaseTools/Source/Python/AutoGen/GenVar.py
>b/BaseTools/Source/Python/AutoGen/GenVar.py
>index 75d455b407..036f00e2bb 100644
>--- a/BaseTools/Source/Python/AutoGen/GenVar.py
>+++ b/BaseTools/Source/Python/AutoGen/GenVar.py
>@@ -20,11 +20,11 @@ import copy
> from Common.VariableAttributes import VariableAttributes
> from Common.Misc import *
> import collections
> import Common.DataType as DataType
>
>-var_info = collections.namedtuple("uefi_var",
>"pcdindex,pcdname,defaultstoragename,skuname,var_name, var_guid,
>var_offset,var_attribute,pcd_default_value, default_value, data_type")
>+var_info = collections.namedtuple("uefi_var",
>"pcdindex,pcdname,defaultstoragename,skuname,var_name, var_guid,
>var_offset,var_attribute,pcd_default_value, default_value,
>data_type,PcdDscLine,StructurePcd")
> NvStorageHeaderSize = 28
> VariableHeaderSize = 32
>
> class VariableMgr(object):
> def __init__(self, DefaultStoreMap, SkuIdMap):
>@@ -54,37 +54,74 @@ class VariableMgr(object):
> value_str = "{"
> default_var_bin_strip = [ data.strip("""'""") for data in 
> default_var_bin]
> value_str += ",".join(default_var_bin_strip)
> value_str += "}"
> return value_str
>+def Do_combine(self,sku_var_info_offset_list):
>+newvalue = {}
>+for item in sku_var_info_offset_list:
>+data_type = item.data_type
>+value_list = item.default_value.strip("{").strip("}").split(",")
>+if data_type in DataType.TAB_PCD_NUMERIC_TYPES:
>+data_flag =
>DataType.PACK_CODE_BY_SIZE[MAX_SIZE_TYPE[data_type]]
>+data = value_list[0]
>+value_list = []
>+for data_byte in pack(data_flag, int(data, 16) if
>data.upper().startswith('0X') else int(data)):
>+value_list.append(hex(unpack("B", data_byte)[0]))
>+newvalue[int(item.var_offset, 16) if
>item.var_offset.upper().startswith("0X") else int(item.var_offset)] =

Re: [edk2] [Patch] INF spec: Correct some items in the Table 1 EDK II [Defines] Section

[Gao, Liming]

Yonghong:
  Please list which items are corrected in the commit message. 

Thanks
Liming
>-Original Message-
>From: Zhu, Yonghong
>Sent: Friday, September 07, 2018 3:08 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch] INF spec: Correct some items in the Table 1 EDK II [Defines]
>Section
>
>Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=1162
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 2_inf_overview/24_[defines]_section.md   | 59 
> 3_edk_ii_inf_file_format/34_[defines]_section.md |  2 +-
> README.md|  1 +
> 3 files changed, 31 insertions(+), 31 deletions(-)
>
>diff --git a/2_inf_overview/24_[defines]_section.md
>b/2_inf_overview/24_[defines]_section.md
>index 37b0135..0afdfed 100644
>--- a/2_inf_overview/24_[defines]_section.md
>+++ b/2_inf_overview/24_[defines]_section.md
>@@ -106,35 +106,34 @@ the PEI Core or the Dxe Core. EDK II only references
>the first possible
> dispatch instance.
> **
>
> ## Table 1 EDK II [Defines] Section Elements
>
>-| Tag  | Required 
>| Value
>| Notes
>|
>-|  | 
>-
>--- | - | 
>--
>---
>---
>---
>---
>- |
>-| `INF_VERSION`| REQUIRED 
>| 1.27 or
>0x0001001B| This identifies the INF spec. version. 
>It is decimal
>value with fraction or two-nibble hexadecimal representation of the same, for
>example: 1.27. Tools use this value to handle parsing of previous releases of
>the specification if there are incompatible changes.
>|
>-| `BASE_NAME`  | REQUIRED 
>| A
>single word | This is a single word identifier 
>that will be used
>for the component name.
>|
>-| `EDK_RELEASE_VERSION`| Not required 
>|
>Hex Double Word   | The minimum revision value 
>across the
>module and all its dependent libraries. If a revision value is not declared in 
>the
>module or any of the dependent libraries, then the tool may use the value of
>0, which disables checking.
>|
>-| `PI_SPECIFICATION_VERSION`   | Not required
>| Decimal or special format of hex  | The minimum revision value 
>across
>the module and all its dependent libraries. If a revision value is not 
>declared in
>the module or any of the dependent libraries, then tools may use the value of
>0, which disables checking.
>|
>-|  |  
>|
>| The `PI_SPECIFICATION_VERSION` must only be set in the INF file if the
>module depends on services or system table fields or PI core behaviors that
>are not present in the PI 1.0 version. For example, if a module depends on
>definitions in PI 1.1 that are not in PI 1.0, then `PI_SPECIFICATION_VERSION`
>must be 0x0001000A
>|
>-| `UEFI_SPECIFICATION_VERSION` | Not required
>| Decimal or special format of hex  | The minimum revision value 
>across
>the module and all its dependent libraries. If a revision value is not 
>declared in
>the module or any of the dependent libraries, then tools may use the value of
>0, which disables checking.
>|
>-|  |  
>|
>| The `UEFI_SPECIFICATION_VERSIon` must only be set in the INF file if the
>module depends on UEFI Boot Services or UEFI Runtime Services or UEFI
>System Table fields or UEFI core behaviors that are not present in the UEFI 2.1
>version. For example, if a module depends on definitions in UEFI 2.2 that are
>not in UEFI 2.1, then `UEFI_SPECIFICATION_VERSION` must be 0x00020014
>|
>-| `FILE_GUID`  | REQUIRED 
>| GUID
>Value| Registry (8-4-4-4-12) Format This 
>value is required for
>all EDK II format INF files, required for EDK driver INF files, not required 
>for
>EDK libraries
>|
>-| `MODULE_TYPE`| REQUIRED  

Re: [edk2] [Patch] Build spec: correct the Operator used in the expression for Table 12

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Friday, September 07, 2018 3:56 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch] Build spec: correct the Operator used in the expression for
>Table 12
>
>Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=598
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> .../82_auto-generation_process.md  | 34 +++---
> README.md  |  1 +
> 2 files changed, 18 insertions(+), 17 deletions(-)
>
>diff --git a/8_pre-build_autogen_stage/82_auto-generation_process.md
>b/8_pre-build_autogen_stage/82_auto-generation_process.md
>index 6ce1710..9b61e0d 100644
>--- a/8_pre-build_autogen_stage/82_auto-generation_process.md
>+++ b/8_pre-build_autogen_stage/82_auto-generation_process.md
>@@ -657,15 +657,11 @@ Refer to the DSC and FDF file form specifications
>"_Conditional Directive
> Blocks_" section for additional details of how directives must be processed.
>
>  8.2.4.6 Expressions
>
> Expressions can be used in conditional directive comparison statements and
>in
>-value fields for PCDs in the DSC and FDF files.
>-
>-**
>-**Note:** Expressions are not supported in the INF and DEC files.
>-**
>+value fields for PCDs in the meta-data files.
>
> Expressions follow C relation, equality, logical and bitwise precedence and
> associativity. Not all C operators are supported, only operators in the
> following list can be used.
>
>@@ -681,22 +677,26 @@ Use of parenthesis is encouraged to remove
>ambiguity.
> Additional scripting style operators may be used in place of C operators as
> shown in the table below.
>
> ## Table 12 Operator Precedence and Supported Operands
>
>-| Operator | Use with Data Types   | Notes
>| Priority |
>-|  | - | 
>--
>---
>---
>- |  |
>-| `or`, `OR`, | Number, Boolean   |
>| Lowest   |
>-| `and`, `AND`, `&&`   | Number, Boolean   |
>|  |
>-|   | Number, Boolean   | 
>Bitwise OR
>|  |
>-| `^`, `xor`, `XOR`| Number, Boolean   | 
>Exclusive OR
>|  |
>-| `&`  | Number, Boolean   | 
>Bitwise AND
>|  |
>-| `==`, `!=`, `EQ`, `NE`, `IN` | All   | The 
>IN operator can only be
>used to test a quoted unary literal string for membership in a list.
>|  |
>-|  |   | 
>Space characters must be used before
>and after the letter operators Strings compared to boolean or numeric values
>using "==" or "EQ" will always return FALSE, while using the "!=" or "NE"
>operators will always return TRUE |  |
>-| `<=`, `>=`, `<`, `>`, `LE`, `GE`, `LT`, `GT` | All   | 
>Space characters must
>be used before and after the letter operators.
>|  |
>-| `+`, `-` | Number, Boolean   | 
>Cannot be used with
>strings - the system does not automatically do concatenation. Tools should
>report a warning message if these operators are used with both a boolean
>and number value  |  |
>-| `!`, `not`, `NOT`| Number, Boolean   |
>| Highest  |
>+| Operator | Use with Data Types | Notes
>| Priority |
>+|  | --- | 
>---
>--   
>-
> |  |
>+| `? :`| All | 
>Conditional operator
>|  Lowest  |
>+| `or`, `OR`, | Number, Boolean |
>|  |
>+| `XOR`, `xor` | Number, Boolean |
>|  |
>+| `and`, `AND`, `&&`   | Number, Boolean |
>|  |
>+|   | Number, Boolean | 
>Bitwise OR
>|  |
>+| `^`  | Number, Boolean | 
>Bitwise XOR
>|  |
>+| `&`  | Number, Boolean | 
>Bitwise AND
>|  |
>+| `==`, `!=`, `EQ`, `NE`, `IN` | All | The IN 
>operator can only be
>used to test a quoted unary literal string for membership in a list.
>|  |
>+|  

[edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position

[Liming Gao]

1. Remove jmp _SmiHandler, and run the code at the same position.
2. Fix up the function call address as the absolute address.
Verify OVMF SMM boot to shell with VS2017, GCC5 and XCODE5 tool chain.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Laszlo Ersek 
Cc: Eric Dong 
Cc: Jiewen Yao 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 34 +
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 315d0f8..d8259de 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -173,9 +173,6 @@ SmiHandlerIdtrAbsAddr:
 mov gs, eax
 mov ax, [rbx + DSC_SS]
 mov ss, eax
-mov rax, strict qword 0 ;   mov rax, _SmiHandler
-_SmiHandlerAbsAddr:
-jmp rax
 
 _SmiHandler:
 mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
@@ -189,13 +186,19 @@ _SmiHandler:
 add rsp, -0x20
 
 mov rcx, rbx
-callASM_PFX(CpuSmmDebugEntry)
+mov rax, strict qword 0 ;   callASM_PFX(CpuSmmDebugEntry)
+CpuSmmDebugEntryAbsAddr:
+callrax
 
 mov rcx, rbx
-callASM_PFX(SmiRendezvous)
+mov rax, strict qword 0 ;   callASM_PFX(SmiRendezvous)
+SmiRendezvousAbsAddr:
+callrax
 
 mov rcx, rbx
-callASM_PFX(CpuSmmDebugExit)
+mov rax, strict qword 0 ;   callASM_PFX(CpuSmmDebugExit)
+CpuSmmDebugExitAbsAddr:
+callrax
 
 add rsp, 0x20
 
@@ -206,7 +209,8 @@ _SmiHandler:
 
 add rsp, 0x200
 
-lea rax, [ASM_PFX(mXdSupported)]
+mov rax, strict qword 0 ;   lea rax, 
[ASM_PFX(mXdSupported)]
+mXdSupportedAbsAddr:
 mov al, [rax]
 cmp al, 0
 jz  .1
@@ -230,7 +234,19 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
 learcx, [SmiHandlerIdtrAbsAddr]
 movqword [rcx - 8], rax
 
-learax, [_SmiHandler]
-learcx, [_SmiHandlerAbsAddr]
+learax, [ASM_PFX(CpuSmmDebugEntry)]
+learcx, [CpuSmmDebugEntryAbsAddr]
+movqword [rcx - 8], rax
+
+learax, [ASM_PFX(SmiRendezvous)]
+learcx, [SmiRendezvousAbsAddr]
+movqword [rcx - 8], rax
+
+learax, [ASM_PFX(CpuSmmDebugExit)]
+learcx, [CpuSmmDebugExitAbsAddr]
+movqword [rcx - 8], rax
+
+learax, [ASM_PFX(mXdSupported)]
+learcx, [mXdSupportedAbsAddr]
 movqword [rcx - 8], rax
 ret
-- 
2.10.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Fix the RaiseError variable issue caused by 855698fb69f

[Chen, Hesheng]

Reviewed-by: Hess Chen 

Chen, Hess
Intel China Software Center
Tel: +86-21-6116-6740
Email: hesheng.c...@intel.com

-Original Message-
From: Zhu, Yonghong 
Sent: Wednesday, September 5, 2018 11:27 AM
To: edk2-devel@lists.01.org
Cc: Chen, Hesheng ; Gao, Liming 
Subject: [Patch] BaseTools: Fix the RaiseError variable issue caused by 
855698fb69f

The bug is that it cause the RaiseError always be set to TRUE even we call the 
function with FALSE parameter.

Cc: Hess Chen 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 BaseTools/Source/Python/Common/EdkLogger.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/Python/Common/EdkLogger.py 
b/BaseTools/Source/Python/Common/EdkLogger.py
index 80697bf..af77074 100644
--- a/BaseTools/Source/Python/Common/EdkLogger.py
+++ b/BaseTools/Source/Python/Common/EdkLogger.py
@@ -196,12 +196,12 @@ def error(ToolName, ErrorCode, Message=None, File=None, 
Line=None, ExtraData=Non
 LogText =  _ErrorMessageTemplate % TemplateDict
 else:
 LogText = _ErrorMessageTemplateWithoutFile % TemplateDict
 
 _ErrorLogger.log(ERROR, LogText)
-RaiseError = IsRaiseError
-if RaiseError:
+
+if RaiseError and IsRaiseError:
 raise FatalError(ErrorCode)
 
 # Log information which should be always put out
 quiet   = _ErrorLogger.error
 
--
2.6.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools: pcd in commandLine.

[Zhu, Yonghong]

The patch's subject is too generic.

Best Regards,
Zhu Yonghong


-Original Message-
From: Zhao, ZhiqiangX 
Sent: Wednesday, September 12, 2018 9:41 AM
To: edk2-devel@lists.01.org
Cc: Zhao, ZhiqiangX ; Gao, Liming 
; Zhu, Yonghong ; Feng, Bob C 

Subject: [PATCH] BaseTools: pcd in commandLine.

In command line, the latter full assign value in commandLine should override 
the former field assign value.
For example, --pcd Token.pcd.field="" --pcd Token.pcd=H"{}".

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: ZhiqiangX Zhao 
Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 17 +
 BaseTools/Source/Python/build/BuildReport.py  | 20 
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index aaef404772..74cb135bbd 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1032,6 +1032,23 @@ class DscBuildData(PlatformBuildClassObject):
 PcdItem = BuildData.Pcds[key]
 if (TokenSpaceGuidCName, TokenCName) == 
(PcdItem.TokenSpaceGuidCName, PcdItem.TokenCName) and FieldName =="":
 PcdItem.DefaultValue = pcdvalue
+#In command line, the latter full assign value in commandLine should 
override the former field assign value.
+#For example, --pcd Token.pcd.field="" --pcd Token.pcd=H"{}"
+delete_assign = []
+field_assign = {}
+if GlobalData.BuildOptionPcd:
+for pcdTuple in GlobalData.BuildOptionPcd:
+TokenSpaceGuid, Token, Field = pcdTuple[0], pcdTuple[1], 
pcdTuple[2]
+if Field:
+if (TokenSpaceGuid, Token) not in field_assign:
+field_assign[TokenSpaceGuid, Token] = []
+field_assign[TokenSpaceGuid, Token].append(pcdTuple)
+else:
+if (TokenSpaceGuid, Token) in field_assign:
+delete_assign.extend(field_assign[TokenSpaceGuid, 
Token])
+field_assign[TokenSpaceGuid, Token] = []
+for item in delete_assign:
+GlobalData.BuildOptionPcd.remove(item)
 
 @staticmethod
 def HandleFlexiblePcd(TokenSpaceGuidCName, TokenCName, PcdValue, 
PcdDatumType, GuidDict, FieldName=''):
diff --git a/BaseTools/Source/Python/build/BuildReport.py 
b/BaseTools/Source/Python/build/BuildReport.py
index a598d64244..3886a7a55e 100644
--- a/BaseTools/Source/Python/build/BuildReport.py
+++ b/BaseTools/Source/Python/build/BuildReport.py
@@ -982,12 +982,16 @@ class PcdReport(object):
 PcdValue = DecDefaultValue
 if DscDefaultValue:
 PcdValue = DscDefaultValue
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the latest, no 
need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 if ModulePcdSet is not None:
 if (Pcd.TokenCName, Pcd.TokenSpaceGuidCName, Type) not in 
ModulePcdSet:
 continue
 InfDefaultValue, PcdValue = ModulePcdSet[Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName, Type]
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the latest, 
no need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 if InfDefaultValue:
 try:
 InfDefaultValue = 
ValueExpressionEx(InfDefaultValue, Pcd.DatumType, self._GuidDict)(True) @@ 
-1003,7 +1007,9 @@ class PcdReport(object):
 if pcd[2]:
 continue
 PcdValue = pcd[3]
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the 
latest, no need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 BuildOptionMatch = True
 break
 
@@ -1050,7 +1056,7 @@ class PcdReport(object):
 DscMatch = (DscDefaultValue.strip() == 
PcdValue.strip())
 
 IsStructure = False
-if GlobalData.gStructurePcd and (self.Arch in 
GlobalData.gStructurePcd) and ((Pcd.TokenCName, Pcd.TokenSpaceGuidCName) in 
GlobalData.gStructurePcd[self.Arch]):
+if self.IsStructurePcd(Pcd.TokenCName, 

[edk2] [PATCH] BaseTools: Correct the SkuOverwrite.

[Zhaozh1x]

StructurePcd, SkuA does not define any structure pcd overwrite,
But SkuA inherit from DEFAULT sku, and DEFAULT sku define
structure pcd overwrite, the pcd value of SkuA should same with
DEFAULT sku.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: ZhiqiangX Zhao 
Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index aaef404772..5321101def 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1838,12 +1838,15 @@ class DscBuildData(PlatformBuildClassObject):
 IsArray = IsFieldValueAnArray(FieldList[FieldName][0])
 if IsArray:
 try:
-FieldList[FieldName][0] = 
ValueExpressionEx(FieldList[FieldName][0], TAB_VOID, self._GuidDict)(True)
+FieldValue = 
ValueExpressionEx(FieldList[FieldName][0], TAB_VOID, self._GuidDict)(True)
 except BadExpression:
 EdkLogger.error('Build', FORMAT_INVALID, "Invalid 
value format for %s. From %s Line %d " %
 
(".".join((Pcd.TokenSpaceGuidCName, Pcd.TokenCName, FieldName)), 
FieldList[FieldName][1], FieldList[FieldName][2]))
 try:
-Value, ValueSize = ParseFieldValue 
(FieldList[FieldName][0])
+if IsArray:
+Value, ValueSize = ParseFieldValue (FieldValue)
+else:
+Value, ValueSize = ParseFieldValue 
(FieldList[FieldName][0])
 except Exception:
 EdkLogger.error('Build', FORMAT_INVALID, "Invalid 
value format for %s. From %s Line %d " % (".".join((Pcd.TokenSpaceGuidCName, 
Pcd.TokenCName, FieldName)), FieldList[FieldName][1], FieldList[FieldName][2]))
 if isinstance(Value, str):
-- 
2.14.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] [edk2-platforms/devel-IntelAtomProcessorE3900] Fixed the klocwork issues.

[Tu, Yunshan]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yunshan Tu 
---
 .../Common/Features/UsbDeviceDxe/XdciDWC.c   | 12 ++--
 .../PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c |  1 +
 .../PlatformBootManagerLib/PlatformBootManager.c |  2 ++
 .../PlatformSettings/PlatformDxe/PciDevice.c |  2 +-
 .../PlatformPreMemPei/BootMode.c |  7 ---
 .../PlatformPreMemPei/FvCallback.c   |  2 +-
 .../Common/PlatformSmm/Platform.c|  2 +-
 .../FspmWrapperPeim/FspmWrapperPeim.c|  3 ++-
 .../NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c  |  4 ++--
 .../Cpu/PowerManagement/Smm/PowerMgmtDts.c   | 16 
 .../Cpu/SmmAccess/Dxe/SmmAccessDriver.c  |  6 +++---
 .../Cpu/SmmAccess/Pei/SmmAccessDriver.c  |  4 ++--
 .../BaseConfigBlockLib/BaseConfigBlockLib.c  |  4 ++--
 .../Library/DxeSmbiosMemoryLib/SmbiosType16.c|  4 ++--
 .../Universal/Variable/RuntimeDxe/Variable.c |  4 ++--
 .../Library/BaseScSpiCommonLib/SpiCommon.c   |  2 +-
 .../SouthCluster/Library/DxeVtdLib/DxeVtdLib.c   |  1 +
 .../Library/Private/DxeScHdaLib/ScHdaLib.c   |  4 +++-
 .../ScPciExpressHelpersLibrary.c |  4 ++--
 .../Library/ScPlatformLib/ScPlatformLibrary.c|  2 +-
 .../BroxtonSiPkg/Txe/Heci/Smm/HeciSmm.c  |  8 
 .../BroxtonSiPkg/Txe/Library/SeCLib/HeciMsgLib.c |  2 +-
 22 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/Platform/BroxtonPlatformPkg/Common/Features/UsbDeviceDxe/XdciDWC.c 
b/Platform/BroxtonPlatformPkg/Common/Features/UsbDeviceDxe/XdciDWC.c
index 2c1e929ab7..2ade4434bd 100644
--- a/Platform/BroxtonPlatformPkg/Common/Features/UsbDeviceDxe/XdciDWC.c
+++ b/Platform/BroxtonPlatformPkg/Common/Features/UsbDeviceDxe/XdciDWC.c
@@ -22,7 +22,7 @@ UsbRegRead (
   IN UINT32Offset
   )
 {
-  volatile UINT32 *addr = (volatile UINT32 *)(UINTN)(Base + Offset);
+  volatile UINT32 *addr = (volatile UINT32 *)((UINTN)(Base) + (UINTN)(Offset));
   return *addr;
 }
 
@@ -33,7 +33,7 @@ UsbRegWrite (
   IN UINT32val
   )
 {
-  volatile UINT32 *addr = (volatile UINT32 *)(UINTN)(Base + Offset);
+  volatile UINT32 *addr = (volatile UINT32 *)((UINTN)(Base) + (UINTN)(Offset));
   *addr = val;
 }
 
@@ -1852,9 +1852,9 @@ DwcXdciCoreInit (
   //
   // Prepare a Buffer for SETUP packet
   //
-  LocalCoreHandle->Trbs = (DWC_XDCI_TRB *)(UINTN)((UINT32)(UINTN)
+  LocalCoreHandle->Trbs = (DWC_XDCI_TRB *)((UINTN)
 LocalCoreHandle->UnalignedTrbs +
-(DWC_XDCI_TRB_BYTE_ALIGNMENT -
+(UINTN)(DWC_XDCI_TRB_BYTE_ALIGNMENT -
 ((UINT32)(UINTN)LocalCoreHandle->UnalignedTrbs %
 DWC_XDCI_TRB_BYTE_ALIGNMENT)));
 
@@ -3954,9 +3954,9 @@ UsbXdciCoreReinit (
   //
   // Prepare a Buffer for SETUP packet
   //
-  LocalCoreHandle->Trbs = (DWC_XDCI_TRB *)(UINTN)((UINT32)(UINTN)
+  LocalCoreHandle->Trbs = (DWC_XDCI_TRB *)((UINTN)
 LocalCoreHandle->UnalignedTrbs +
-(DWC_XDCI_TRB_BYTE_ALIGNMENT -
+(UINTN)(DWC_XDCI_TRB_BYTE_ALIGNMENT -
 ((UINT32)(UINTN)LocalCoreHandle->UnalignedTrbs %
 DWC_XDCI_TRB_BYTE_ALIGNMENT)));
 
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c
 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c
index c404116029..7bf54ca37d 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Library/PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Library/PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c
@@ -89,6 +89,7 @@ PeiFspCpuPolicyInit (
 
   VariableSize = sizeof (SYSTEM_CONFIGURATION);
   SystemConfiguration = AllocateZeroPool (VariableSize);
+  ASSERT (SystemConfiguration != NULL);
 
   Status = VariableServices->GetVariable (
VariableServices,
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Library/PlatformBootManagerLib/PlatformBootManager.c
 
b/Platform/BroxtonPlatformPkg/Common/Library/PlatformBootManagerLib/PlatformBootManager.c
index 5201ac080d..821ed95bc4 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Library/PlatformBootManagerLib/PlatformBootManager.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Library/PlatformBootManagerLib/PlatformBootManager.c
@@ -524,6 +524,8 @@ ProcessTcgPp (
   EFI_TCG2_PHYSICAL_PRESENCE Tcg2PpData;
   EFI_PHYSICAL_PRESENCE  TcgPpData;
   UINTN  TcgPpDataSize;
+  
+  Status = EFI_SUCCESS;
 
   TcgPpData.PPRequest = TCG_PHYSICAL_PRESENCE_NO_ACTION;
   Tcg2PpData.PPRequest = TCG2_PHYSICAL_PRESENCE_NO_ACTION;
diff --git 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformDxe/PciDevice.c 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformDxe/PciDevice.c
index 

Re: [edk2] [Patch] BaseTools: Report error for incorrect hex value format

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong 
Zhu
Sent: Tuesday, September 11, 2018 10:51 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: [edk2] [Patch] BaseTools: Report error for incorrect hex value format

From: zhijufan 

The case is user use 0x1} as a hex value for Pcd, it directly cause tool report 
traceback info. This patch add more error info.

Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/Common/Misc.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/Common/Misc.py 
b/BaseTools/Source/Python/Common/Misc.py
index 2cf9574..430bf6b 100644
--- a/BaseTools/Source/Python/Common/Misc.py
+++ b/BaseTools/Source/Python/Common/Misc.py
@@ -1412,11 +1412,14 @@ def ParseFieldValue (Value):
 if Value.startswith('DEVICE_PATH(') and Value.endswith(')'):
 Value = Value.replace("DEVICE_PATH(", '').rstrip(')')
 Value = Value.strip().strip('"')
 return ParseDevPathValue(Value)
 if Value.lower().startswith('0x'):
-Value = int(Value, 16)
+try:
+Value = int(Value, 16)
+except:
+raise BadExpression("invalid hex value: %s" % Value)
 if Value == 0:
 return 0, 1
 return Value, (Value.bit_length() + 7) / 8
 if Value[0].isdigit():
 Value = int(Value, 10)
--
2.6.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value

[Ni, Ruiyu]

Mike,
Do you require to still use MSVC intrinsic function?

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni,
> Ruiyu
> Sent: Tuesday, September 11, 2018 10:27 AM
> To: Kinney, Michael D ; 'edk2-devel@lists.01.org'
> 
> Cc: Yao, Jiewen ; Gao, Liming 
> Subject: Re: [edk2] [PATCH] MdePkg/SynchronizationLib: fix
> Interlocked[De|In]crement return value
> 
> The reason I didn't remove the GCC version is because Liming told me that 
> there
> is a XCODE issue which prevents using NASM for library.
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Tuesday, September 11, 2018 10:25 AM
> > To: Kinney, Michael D ; edk2-
> > de...@lists.01.org
> > Cc: Yao, Jiewen ; Gao, Liming
> > 
> > Subject: RE: [PATCH] MdePkg/SynchronizationLib: fix
> > Interlocked[De|In]crement return value
> >
> > The NASM does the exactly same as the MSFT intrinsic.
> > I'd like to remove them because I was almost lost in so many versions
> > of Interlocked[De|In]crement.
> > I'd like to merge them to one version.
> >
> > Thanks/Ray
> >
> > > -Original Message-
> > > From: Kinney, Michael D
> > > Sent: Tuesday, September 11, 2018 12:39 AM
> > > To: Ni, Ruiyu ; edk2-devel@lists.01.org; Kinney,
> > > Michael D 
> > > Cc: Yao, Jiewen ; Gao, Liming
> > > 
> > > Subject: RE: [PATCH] MdePkg/SynchronizationLib: fix
> > > Interlocked[De|In]crement return value
> > >
> > > Ray,
> > >
> > > Why are we removing the use of intrinsics from MSFT builds?  We
> > > should keep those for more efficient code generation if the
> > > intrinsic has the correct behavior.
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > > > -Original Message-
> > > > From: Ni, Ruiyu
> > > > Sent: Monday, September 10, 2018 3:06 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Yao, Jiewen ; Gao, Liming
> > > > ; Kinney, Michael D
> > > > 
> > > > Subject: [PATCH] MdePkg/SynchronizationLib: fix
> > > > Interlocked[De|In]crement return value
> > > >
> > > > Today's InterlockedIncrement()/InterlockedDecrement()
> > > > guarantees to
> > > > perform atomic increment/decrement but doesn't guarantee the
> > > > return value equals to the new value.
> > > >
> > > > The patch fixes the behavior to use "XADD" instruction to
> > > > guarantee the return value equals to the new value.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Ruiyu Ni 
> > > > Cc: Jiewen Yao 
> > > > Cc: Liming Gao 
> > > > Cc: Michael D Kinney 
> > > > ---
> > > >  MdePkg/Include/Library/SynchronizationLib.h|  6
> > > > +--
> > > >  .../BaseSynchronizationLib.inf | 18
> > > > -
> > > >  .../BaseSynchronizationLibInternals.h  |  6
> > > > +--
> > > >  .../BaseSynchronizationLib/Ia32/GccInline.c| 18
> > > > -
> > > >  .../Ia32/InterlockedDecrement.c| 42
> > > > 
> > > >  .../Ia32/InterlockedDecrement.nasm | 10
> > > > ++---
> > > >  .../Ia32/InterlockedIncrement.c| 43
> > > > 
> > > >  .../Ia32/InterlockedIncrement.nasm |  7
> > > > ++--
> > > >  .../BaseSynchronizationLib/Synchronization.c   |  6
> > > > +--
> > > >  .../BaseSynchronizationLib/SynchronizationGcc.c|  6
> > > > +--
> > > >  .../BaseSynchronizationLib/SynchronizationMsc.c|  6
> > > > +--
> > > >  .../Library/BaseSynchronizationLib/X64/GccInline.c | 19
> > > > +
> > > >  .../X64/InterlockedDecrement.c | 46
> > > > --
> > > >  .../X64/InterlockedDecrement.nasm  |  8
> > > > ++--
> > > >  .../X64/InterlockedIncrement.c | 46
> > > > --
> > > >  .../X64/InterlockedIncrement.nasm  |  5
> > > > ++-
> > > >  16 files changed, 52 insertions(+), 240 deletions(-)  delete mode
> > > > 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDe
> > > > crement.c
> > > >  delete mode 100644
> > > > MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIn
> > > > crement.c
> > > >  delete mode 100644
> > > > MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDec
> > > > rement.c
> > > >  delete mode 100644
> > > > MdePkg/Library/BaseSynchronizationLib/X64/InterlockedInc
> > > > rement.c
> > > >
> > > > diff --git a/MdePkg/Include/Library/SynchronizationLib.h
> > > > b/MdePkg/Include/Library/SynchronizationLib.h
> > > > index da69f6ff5e..ce3bce04f5 100644
> > > > --- a/MdePkg/Include/Library/SynchronizationLib.h
> > > > +++ b/MdePkg/Include/Library/SynchronizationLib.h
> > > > @@ -144,8 +144,7 @@ ReleaseSpinLock (
> > > >
> > > >Performs an atomic increment of the 32-bit unsigned integer
> > > > specified by
> > > >Value and returns the incremented value. The increment
> > > > operation must be
> > > > -  performed using MP safe mechanisms. The state of the return
> > > > value is not
> > > > -  guaranteed to 

Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack

[Wang, Jian J]

Hi Laszlo and Ard,

Retiring the PcdSetNxForStack is not the intention of this patch series. It's 
just
a side effect of solving problem stated in BZ#1116. Sorry again for the 
misleading
title. I'm not insisting that we have to remove this PCD anyway (My personal
opinion. Others might have different ones). 

I think I understand the importance of PcdSetNxForStack to arm/aarch64 now. And 
I
agree that it'd be better to enable NX for stack independent of enabling NX for
EfiBootServcesData memory. But since the stack is type of EfiBootServicesData,
I think we should avoid any confusion in enabling/disabling NX for them. That's 
what
BZ#1116 tries to complain about. But I'm still not clear any concrete 
suggestion on
how to solve the BZ#1116 from all comment so far, if my patch series cannot 
satisfy
all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think. (It 
doesn't
imply we must remove it.)

As I commented in the BZ#1116, maybe we can just simply assert if there's one
trying to disable NX for stack but enable NX for EfiBootServicesData at the 
same time,
because it doesn't make sense. I think all other setting combinations won't have
such confusion and don't need to take care specifically.

And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs have 
bits
set. Current DxeIpl will just enable NX for PcdSetNxForStack only.

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, September 11, 2018 11:53 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Justen, Jordan L 
; Yao, Jiewen ; Zeng, Star 
; Ard Biesheuvel 
Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of 
PcdSetNxForStack

On 09/11/18 07:16, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since PcdSetNxForStack is expired, remove related config code.
> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
> There's no need to add it in code to replace PcdSetNxForStack.
>
> Cc: Laszlo Ersek 
> Cc: Star Zeng 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Ruiyu Ni 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  OvmfPkg/PlatformPei/Platform.c  | 1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126..6627d236e0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>)
>  {
>UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
> -  UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
>  }
>
>  VOID
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 9c5ad9961c..ef5dcb7693 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,7 +96,6 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>

I disagree with this change. I explained my reasons in
, but for the
sake of discussion, I'll state the same here:

> Ard's got a point here. Just because one PCD implies another, the
> reverse is not necessarily true.
>
> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
> then we had to make it conditional, due to old GRUB variants breaking
> with a non-executable stack. (Please refer to commit d20b06a3afdf,
> "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15).
> Therefore we now default to FALSE, and let the user set it to TRUE on
> the QEMU command line.
>
> In addition, we intentionally inherit a zero
> PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec".
>
> Both of the above configurations satisfy the requirement
>
>   ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> (1 << EfiBootServicesData)) == 0 ||
>PcdGetBool (PcdSetNxForStack))
>
> i.e. the requirement that "NX for BS data" imply "NX for stack".
>
> If you remove the standalone knob for PcdSetNxForStack, then the
> default behavior of OVMF will continue to work, however the command
> line option, for setting PcdSetNxForStack *only*, will break.

I'd like to add another comment just here (not mentioned in the BZ):

To my understanding, the Heap Guard is a debug feature [1]. Over time,
I've reviewed and regression-tested all the Heap Guard patches that have
crossed my path with the understanding that "this is not enabled by
default in OVMF". With those points in mind, I certainly don't intend to
enable the Heap Guard as a FixedPCD -- 

Re: [edk2] [patch 3/3] MdeModulePkg: Avoid key notification called more than once

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Bi, Dandan 
Sent: Monday, September 10, 2018 3:12 PM
To: edk2-devel@lists.01.org
Cc: Bi, Dandan ; Ni, Ruiyu ; Zeng, 
Star 
Subject: [patch 3/3] MdeModulePkg: Avoid key notification called more than once

From: Dandan Bi 

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

Issue:
In current code logic, when a key is pressed, it will search the whole 
NotifyList to find whether a notification has been registered with the 
keystroke. if yes, it will en-queue the key for notification execution later. 
And now if different notification functions have been registered with the same 
key, then the key will be en-queued more than once. Then it will cause the 
notification executed more than once.

This patch is to enhance the code logic to fix this issue.

Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c| 1 +
 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c   | 1 +
 MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c 
b/MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c
index 2ee293d64d..53cb6f0b48 100644
--- a/MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c
+++ b/MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c
@@ -1449,10 +1449,11 @@ KeyGetchar (
   // while current TPL is TPL_NOTIFY. It will be invoked in
   // KeyNotifyProcessHandler() which runs at TPL_CALLBACK.
   //
   PushEfikeyBufTail (>EfiKeyQueueForNotify, );
   gBS->SignalEvent (ConsoleIn->KeyNotifyProcessEvent);
+  break;
 }
   }
 
   PushEfikeyBufTail (>EfiKeyQueue, );  } diff --git 
a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c 
b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
index b3b5fb9ff4..9cb4b5db6b 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -1693,10 +1693,11 @@ UsbKeyCodeToEfiInputKey (
   // while current TPL is TPL_NOTIFY. It will be invoked in
   // KeyNotifyProcessHandler() which runs at TPL_CALLBACK.
   //
   Enqueue (>EfiKeyQueueForNotify, KeyData, sizeof 
(*KeyData));
   gBS->SignalEvent (UsbKeyboardDevice->KeyNotifyProcessEvent);
+  break;
 }
   }
 
   return EFI_SUCCESS;
 }
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c 
b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 33f9b6e585..d681a3039e 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
@@ -985,10 +985,11 @@ EfiKeyFiFoInsertOneKey (
   // while current TPL is TPL_NOTIFY. It will be invoked in
   // KeyNotifyProcessHandler() which runs at TPL_CALLBACK.
   //
   EfiKeyFiFoForNotifyInsertOneKey (TerminalDevice->EfiKeyFiFoForNotify, 
Key);
   gBS->SignalEvent (TerminalDevice->KeyNotifyProcessEvent);
+  break;
 }
   }
   if (IsEfiKeyFiFoFull (TerminalDevice)) {
 //
 // Efi Key FIFO is full
--
2.14.3.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] BaseTools: Align the boolean type PCD value's display in the report

[Yonghong Zhu]

From: zhijufan 

This patch align the boolean type PCD value's display in the build
report. Original it may display 0x0, also may use 0 for the same PCD.

Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/build/BuildReport.py | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/Python/build/BuildReport.py 
b/BaseTools/Source/Python/build/BuildReport.py
index a598d64..c7fa1b9 100644
--- a/BaseTools/Source/Python/build/BuildReport.py
+++ b/BaseTools/Source/Python/build/BuildReport.py
@@ -1012,11 +1012,11 @@ class PcdReport(object):
 FileWrite(File, "")
 FileWrite(File, Key)
 First = False
 
 
-if Pcd.DatumType in TAB_PCD_CLEAN_NUMERIC_TYPES:
+if Pcd.DatumType in TAB_PCD_NUMERIC_TYPES:
 PcdValueNumber = int(PcdValue.strip(), 0)
 if DecDefaultValue is None:
 DecMatch = True
 else:
 DecDefaultValueNumber = int(DecDefaultValue.strip(), 0)
@@ -1100,10 +1100,19 @@ class PcdReport(object):
 DecMatch = False
 
 #
 # Report PCD item according to their override relationship
 #
+if Pcd.DatumType == 'BOOLEAN':
+if DscDefaultValue:
+DscDefaultValue = str(int(DscDefaultValue, 0))
+if DecDefaultValue:
+DecDefaultValue = str(int(DecDefaultValue, 0))
+if InfDefaultValue:
+InfDefaultValue = str(int(InfDefaultValue, 0))
+if Pcd.DefaultValue:
+Pcd.DefaultValue = str(int(Pcd.DefaultValue, 0))
 if DecMatch:
 self.PrintPcdValue(File, Pcd, PcdTokenCName, TypeName, 
IsStructure, DscMatch, DscDefaultValBak, InfMatch, InfDefaultValue, DecMatch, 
DecDefaultValue, '  ')
 elif InfDefaultValue and InfMatch:
 self.PrintPcdValue(File, Pcd, PcdTokenCName, TypeName, 
IsStructure, DscMatch, DscDefaultValBak, InfMatch, InfDefaultValue, DecMatch, 
DecDefaultValue, '*M')
 elif BuildOptionMatch:
@@ -1124,13 +1133,15 @@ class PcdReport(object):
 continue
 if not BuildOptionMatch:
 ModuleOverride = 
self.ModulePcdOverride.get((Pcd.TokenCName, Pcd.TokenSpaceGuidCName), {})
 for ModulePath in ModuleOverride:
 ModuleDefault = ModuleOverride[ModulePath]
-if Pcd.DatumType in TAB_PCD_CLEAN_NUMERIC_TYPES:
+if Pcd.DatumType in TAB_PCD_NUMERIC_TYPES:
 ModulePcdDefaultValueNumber = 
int(ModuleDefault.strip(), 0)
 Match = (ModulePcdDefaultValueNumber == 
PcdValueNumber)
+if Pcd.DatumType == 'BOOLEAN':
+ModuleDefault = 
str(ModulePcdDefaultValueNumber)
 else:
 Match = (ModuleDefault.strip() == 
PcdValue.strip())
 if Match:
 continue
 IsByteArray, ArrayList = 
ByteArrayForamt(ModuleDefault.strip())
@@ -1245,10 +1256,12 @@ class PcdReport(object):
 if SkuInfo.DefaultStoreDict:
 DefaultStoreList = 
sorted(SkuInfo.DefaultStoreDict.keys())
 for DefaultStore in DefaultStoreList:
 Value = SkuInfo.DefaultStoreDict[DefaultStore]
 IsByteArray, ArrayList = ByteArrayForamt(Value)
+if Pcd.DatumType == 'BOOLEAN':
+Value = str(int(Value, 0))
 if FirstPrint:
 FirstPrint = False
 if IsByteArray:
 if self.DefaultStoreSingle and 
self.SkuSingle:
 FileWrite(File, ' %-*s   : %6s %10s = 
%s' % (self.MaxLen, Flag + ' ' + PcdTokenCName, TypeName, '(' + Pcd.DatumType + 
')', '{'))
@@ -1307,10 +1320,12 @@ class PcdReport(object):
 self.PrintStructureInfo(File, 
OverrideFieldStruct)
 self.PrintPcdDefault(File, Pcd, IsStructure, 
DscMatch, DscDefaultValue, InfMatch, InfDefaultValue, DecMatch, DecDefaultValue)
 else:
 Value = SkuInfo.DefaultValue
 IsByteArray, ArrayList = ByteArrayForamt(Value)
+if Pcd.DatumType == 'BOOLEAN':
+Value = str(int(Value, 

Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Ni, Ruiyu 
Sent: Monday, September 10, 2018 6:05 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Kinney, Michael D 
; Zeng, Star 
Subject: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

Today's implementation does an exact device path match to check whether the 
device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to carry all device 
paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from 
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across boot, the 
USB keyboard doesn't work in the second boot.

ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform doesn't care 
whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control whether 
to enable USB keyboard as an active console.

The patch changes ConPlatform to support USB short-form device path when 
checking whether the device path of a console is in ConIn/ConOut/ErrOut.

The logic to always accept USB/PCCARD device as active console is removed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Hao A Wu 
Cc: Michael D Kinney 
Cc: Star Zeng 
---
 .../Universal/Console/ConPlatformDxe/ConPlatform.c | 513 ++--- 
 .../Universal/Console/ConPlatformDxe/ConPlatform.h |  24 +-
 .../Console/ConPlatformDxe/ConPlatformDxe.inf  |   1 +
 3 files changed, 339 insertions(+), 199 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c 
b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
index 5fa7facfca..27df8a4b56 100644
--- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
+++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
@@ -202,8 +202,7 @@ ConPlatformDriverBindingSupported (
   Start this driver on ControllerHandle by opening Simple Text Input Protocol,
   reading Device Path, and installing Console In Devcice GUID on 
ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConInDev.
+  Append its device path into the console environment variables ConInDev.
 
   @param  This Protocol instance pointer.
   @param  ControllerHandle Handle of device to bind driver to
@@ -270,58 +269,32 @@ ConPlatformTextInDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConInDev, and install
-  // gEfiConsoleInDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // Append the device path to the ConInDev environment variable
   //
-  if (IsHotPlugDevice (DevicePath)) {
+  ConPlatformUpdateDeviceVariable (
+L"ConInDev",
+DevicePath,
+Append
+);
+
+  //
+  // If the device path is an instance in the ConIn environment 
+ variable,  // then install EfiConsoleInDeviceGuid onto 
+ ControllerHandle  //  if (IsInConInVariable) {
 gBS->InstallMultipleProtocolInterfaces (
,
,
NULL,
NULL
);
-//
-// Append the device path to ConInDev only if it is in ConIn variable.
-//
-if (IsInConInVariable) {
-  ConPlatformUpdateDeviceVariable (
-L"ConInDev",
-DevicePath,
-Append
-);
-}
   } else {
-//
-// If it is not a hot-plug device, append the device path to the
-// ConInDev environment variable
-//
-ConPlatformUpdateDeviceVariable (
-  L"ConInDev",
-  DevicePath,
-  Append
-  );
-
-//
-// If the device path is an instance in the ConIn environment variable,
-// then install EfiConsoleInDeviceGuid onto ControllerHandle
-//
-if (IsInConInVariable) {
-  gBS->InstallMultipleProtocolInterfaces (
- ,
- ,
- NULL,
- NULL
- );
-} else {
-  gBS->CloseProtocol (
- ControllerHandle,
- ,
- This->DriverBindingHandle,
- ControllerHandle
- );
-}
+gBS->CloseProtocol (
+   ControllerHandle,
+   ,
+   This->DriverBindingHandle,
+   ControllerHandle
+   );
   }
 
   return EFI_SUCCESS;
@@ -334,8 +307,7 @@ ConPlatformTextInDriverBindingStart (
   reading Device Path, and installing Console Out Devcic GUID, Standard Error
   Device GUID on ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConOutDev, ErrOutDev.
+  Append its device path into the console environment variables ConOutDev, 
ErrOutDev.
 
   @param  

[edk2] [PATCH] BaseTools: pcd in commandLine.

[Zhaozh1x]

In command line, the latter full assign value in commandLine
should override the former field assign value.
For example, --pcd Token.pcd.field="" --pcd Token.pcd=H"{}".

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: ZhiqiangX Zhao 
Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 17 +
 BaseTools/Source/Python/build/BuildReport.py  | 20 
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index aaef404772..74cb135bbd 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1032,6 +1032,23 @@ class DscBuildData(PlatformBuildClassObject):
 PcdItem = BuildData.Pcds[key]
 if (TokenSpaceGuidCName, TokenCName) == 
(PcdItem.TokenSpaceGuidCName, PcdItem.TokenCName) and FieldName =="":
 PcdItem.DefaultValue = pcdvalue
+#In command line, the latter full assign value in commandLine should 
override the former field assign value.
+#For example, --pcd Token.pcd.field="" --pcd Token.pcd=H"{}"
+delete_assign = []
+field_assign = {}
+if GlobalData.BuildOptionPcd:
+for pcdTuple in GlobalData.BuildOptionPcd:
+TokenSpaceGuid, Token, Field = pcdTuple[0], pcdTuple[1], 
pcdTuple[2]
+if Field:
+if (TokenSpaceGuid, Token) not in field_assign:
+field_assign[TokenSpaceGuid, Token] = []
+field_assign[TokenSpaceGuid, Token].append(pcdTuple)
+else:
+if (TokenSpaceGuid, Token) in field_assign:
+delete_assign.extend(field_assign[TokenSpaceGuid, 
Token])
+field_assign[TokenSpaceGuid, Token] = []
+for item in delete_assign:
+GlobalData.BuildOptionPcd.remove(item)
 
 @staticmethod
 def HandleFlexiblePcd(TokenSpaceGuidCName, TokenCName, PcdValue, 
PcdDatumType, GuidDict, FieldName=''):
diff --git a/BaseTools/Source/Python/build/BuildReport.py 
b/BaseTools/Source/Python/build/BuildReport.py
index a598d64244..3886a7a55e 100644
--- a/BaseTools/Source/Python/build/BuildReport.py
+++ b/BaseTools/Source/Python/build/BuildReport.py
@@ -982,12 +982,16 @@ class PcdReport(object):
 PcdValue = DecDefaultValue
 if DscDefaultValue:
 PcdValue = DscDefaultValue
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the latest, no 
need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 if ModulePcdSet is not None:
 if (Pcd.TokenCName, Pcd.TokenSpaceGuidCName, Type) not in 
ModulePcdSet:
 continue
 InfDefaultValue, PcdValue = ModulePcdSet[Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName, Type]
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the latest, 
no need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 if InfDefaultValue:
 try:
 InfDefaultValue = 
ValueExpressionEx(InfDefaultValue, Pcd.DatumType, self._GuidDict)(True)
@@ -1003,7 +1007,9 @@ class PcdReport(object):
 if pcd[2]:
 continue
 PcdValue = pcd[3]
-Pcd.DefaultValue = PcdValue
+#The DefaultValue of StructurePcd already be the 
latest, no need to update.
+if not self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
+Pcd.DefaultValue = PcdValue
 BuildOptionMatch = True
 break
 
@@ -1050,7 +1056,7 @@ class PcdReport(object):
 DscMatch = (DscDefaultValue.strip() == 
PcdValue.strip())
 
 IsStructure = False
-if GlobalData.gStructurePcd and (self.Arch in 
GlobalData.gStructurePcd) and ((Pcd.TokenCName, Pcd.TokenSpaceGuidCName) in 
GlobalData.gStructurePcd[self.Arch]):
+if self.IsStructurePcd(Pcd.TokenCName, 
Pcd.TokenSpaceGuidCName):
 IsStructure = True
 if TypeName in ('DYNVPD', 'DEXVPD'):
 SkuInfoList = Pcd.SkuInfoList
@@ -1413,6 +1419,12 @@ class PcdReport(object):
 else:
 return value
 
+def IsStructurePcd(self, PcdToken, 

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Yao, Jiewen]

We have some internal feature, required to run the code at the each SMI 
entrypoint, instead of common code.
That is why we write code in previous way.

I suggest we keep using the previous way and provide a good example on how to 
handle absolute address in new way.

Can we provide an update for below:
> >mov   rax, ASM_PFX(CpuSmmDebugEntry)
> >callrax

Thank you
Yao Jiewen

> -Original Message-
> From: Gao, Liming
> Sent: Wednesday, September 12, 2018 9:30 AM
> To: Yao, Jiewen ; Laszlo Ersek ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Gao, Liming 
> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> unnecessary jmp _SmiHandler
> 
> Jiewen:
>   Because nasm doesn't generate the absolute address, we can change the
> below code, but we need to fix up its value at boot time like current
> _SmiHandler way.
> 
>   Could you let me know why can't use current way?
> 
> Thanks
> Liming
> >-Original Message-
> >From: Yao, Jiewen
> >Sent: Wednesday, September 12, 2018 9:04 AM
> >To: Gao, Liming ; Laszlo Ersek
> ;
> >edk2-devel@lists.01.org
> >Cc: Dong, Eric 
> >Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >unnecessary jmp _SmiHandler
> >
> >The original code is below. Can we rollback to the indirect call?
> >
> >mov   rax, ASM_PFX(CpuSmmDebugEntry)
> >callrax
> >
> >Thank you
> >Yao Jiewen
> >
> >> -Original Message-
> >> From: Gao, Liming
> >> Sent: Wednesday, September 12, 2018 8:59 AM
> >> To: Yao, Jiewen ; Laszlo Ersek
> >;
> >> edk2-devel@lists.01.org
> >> Cc: Dong, Eric 
> >> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >> unnecessary jmp _SmiHandler
> >>
> >> Jiewen:
> >>   After do more verification, I recall this change. Current code is really
> >> required. Without it, OVMF SMM can't boot. So, below code can't be
> >> removed.
> >>   The reason is that nasm _SmiEntryPoint() function is copied to another
> >> memory location and run. But, _ SmiEntryPoint() calls the external C
> >function
> >> CpuSmmDebugEntry(). nasm compiler generates the function call with the
> >> relative address. After _SmiEntryPoint() function is copied and run in the
> >new
> >> address, its external function call will not work. To fix it, I add jmp
> instruction
> >> to the original address, then process function all and works.
> >>
> >> mov rax, strict qword 0 ;   mov rax,
> _SmiHandler
> >> _SmiHandlerAbsAddr:
> >> jmp rax
> >>
> >> ...
> >> mov rcx, rbx
> >> callASM_PFX(CpuSmmDebugEntry)
> >>
> >> Thanks
> >> Liming
> >> >-Original Message-
> >> >From: Yao, Jiewen
> >> >Sent: Wednesday, September 12, 2018 6:03 AM
> >> >To: Laszlo Ersek ; Gao, Liming
> >> ;
> >> >edk2-devel@lists.01.org
> >> >Cc: Dong, Eric 
> >> >Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >> >unnecessary jmp _SmiHandler
> >> >
> >> >HI
> >> >Would you please add info on what unit test has been done?
> >> >
> >> >Thank you
> >> >Yao Jiewen
> >> >
> >> >
> >> >> -Original Message-
> >> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >Of
> >> >> Laszlo Ersek
> >> >> Sent: Tuesday, September 11, 2018 11:06 PM
> >> >> To: Gao, Liming ; edk2-devel@lists.01.org
> >> >> Cc: Yao, Jiewen ; Dong, Eric
> >> 
> >> >> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >> >> unnecessary jmp _SmiHandler
> >> >>
> >> >> On 09/10/18 10:20, Liming Gao wrote:
> >> >> > This change is wrong introduced by
> >> >> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
> >> >> > It is not required. So, revert it.
> >> >> >
> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> >> > Signed-off-by: Liming Gao 
> >> >> > Cc: Jiewen Yao 
> >> >> > ---
> >> >> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
> >> >> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >> >> >
> >> >> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> >> > index 315d0f8..7b1b3ca 100644
> >> >> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> >> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> >> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
> >> >> >  mov gs, eax
> >> >> >  mov ax, [rbx + DSC_SS]
> >> >> >  mov ss, eax
> >> >> > -mov rax, strict qword 0 ;   mov rax,
> >> >> _SmiHandler
> >> >> > -_SmiHandlerAbsAddr:
> >> >> > -jmp rax
> >> >> > +
> >> >> > +;   jmp _SmiHandler ; instruction is not
> >> needed
> >> >> >
> >> >> >  _SmiHandler:
> >> >> >  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
> >> >> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
> >> >> >  learax, [ASM_PFX(gSmiHandlerIdtr)]
> >> >> >  learcx, [SmiHandlerIdtrAbsAddr]
> >> >> >  movqword [rcx - 8], rax
> >> >> > -
> >> >> > -learax, [_SmiHandler]
> >> >> > -learcx, [_SmiHandlerAbsAddr]
> 

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Gao, Liming]

Jiewen:
  Because nasm doesn't generate the absolute address, we can change the below 
code, but we need to fix up its value at boot time like current _SmiHandler 
way. 

  Could you let me know why can't use current way? 

Thanks
Liming
>-Original Message-
>From: Yao, Jiewen
>Sent: Wednesday, September 12, 2018 9:04 AM
>To: Gao, Liming ; Laszlo Ersek ;
>edk2-devel@lists.01.org
>Cc: Dong, Eric 
>Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>unnecessary jmp _SmiHandler
>
>The original code is below. Can we rollback to the indirect call?
>
>mov   rax, ASM_PFX(CpuSmmDebugEntry)
>callrax
>
>Thank you
>Yao Jiewen
>
>> -Original Message-
>> From: Gao, Liming
>> Sent: Wednesday, September 12, 2018 8:59 AM
>> To: Yao, Jiewen ; Laszlo Ersek
>;
>> edk2-devel@lists.01.org
>> Cc: Dong, Eric 
>> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> unnecessary jmp _SmiHandler
>>
>> Jiewen:
>>   After do more verification, I recall this change. Current code is really
>> required. Without it, OVMF SMM can't boot. So, below code can't be
>> removed.
>>   The reason is that nasm _SmiEntryPoint() function is copied to another
>> memory location and run. But, _ SmiEntryPoint() calls the external C
>function
>> CpuSmmDebugEntry(). nasm compiler generates the function call with the
>> relative address. After _SmiEntryPoint() function is copied and run in the
>new
>> address, its external function call will not work. To fix it, I add jmp 
>> instruction
>> to the original address, then process function all and works.
>>
>> mov rax, strict qword 0 ;   mov rax, _SmiHandler
>> _SmiHandlerAbsAddr:
>> jmp rax
>>
>> ...
>> mov rcx, rbx
>> callASM_PFX(CpuSmmDebugEntry)
>>
>> Thanks
>> Liming
>> >-Original Message-
>> >From: Yao, Jiewen
>> >Sent: Wednesday, September 12, 2018 6:03 AM
>> >To: Laszlo Ersek ; Gao, Liming
>> ;
>> >edk2-devel@lists.01.org
>> >Cc: Dong, Eric 
>> >Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> >unnecessary jmp _SmiHandler
>> >
>> >HI
>> >Would you please add info on what unit test has been done?
>> >
>> >Thank you
>> >Yao Jiewen
>> >
>> >
>> >> -Original Message-
>> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
>Of
>> >> Laszlo Ersek
>> >> Sent: Tuesday, September 11, 2018 11:06 PM
>> >> To: Gao, Liming ; edk2-devel@lists.01.org
>> >> Cc: Yao, Jiewen ; Dong, Eric
>> 
>> >> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> >> unnecessary jmp _SmiHandler
>> >>
>> >> On 09/10/18 10:20, Liming Gao wrote:
>> >> > This change is wrong introduced by
>> >> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
>> >> > It is not required. So, revert it.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Liming Gao 
>> >> > Cc: Jiewen Yao 
>> >> > ---
>> >> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
>> >> >  1 file changed, 2 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > index 315d0f8..7b1b3ca 100644
>> >> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
>> >> >  mov gs, eax
>> >> >  mov ax, [rbx + DSC_SS]
>> >> >  mov ss, eax
>> >> > -mov rax, strict qword 0 ;   mov rax,
>> >> _SmiHandler
>> >> > -_SmiHandlerAbsAddr:
>> >> > -jmp rax
>> >> > +
>> >> > +;   jmp _SmiHandler ; instruction is not
>> needed
>> >> >
>> >> >  _SmiHandler:
>> >> >  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
>> >> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>> >> >  learax, [ASM_PFX(gSmiHandlerIdtr)]
>> >> >  learcx, [SmiHandlerIdtrAbsAddr]
>> >> >  movqword [rcx - 8], rax
>> >> > -
>> >> > -learax, [_SmiHandler]
>> >> > -learcx, [_SmiHandlerAbsAddr]
>> >> > -movqword [rcx - 8], rax
>> >> >  ret
>> >> >
>> >>
>> >> Please remember to CC package maintainers / reviewers directly.
>> >>
>> >> The patch makes sense to me, and indeed it restores the original code.
>> >>
>> >> Reviewed-by: Laszlo Ersek 
>> >>
>> >> Can you perhaps add another sentence to the commit message, before
>> you
>> >> push the patch, such as "the original code already uses RIP-relative
>> >> addressing"?
>> >>
>> >> Thanks
>> >> Laszlo
>> >> ___
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Yao, Jiewen]

The original code is below. Can we rollback to the indirect call?

mov   rax, ASM_PFX(CpuSmmDebugEntry)
callrax

Thank you
Yao Jiewen

> -Original Message-
> From: Gao, Liming
> Sent: Wednesday, September 12, 2018 8:59 AM
> To: Yao, Jiewen ; Laszlo Ersek ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> unnecessary jmp _SmiHandler
> 
> Jiewen:
>   After do more verification, I recall this change. Current code is really
> required. Without it, OVMF SMM can't boot. So, below code can't be
> removed.
>   The reason is that nasm _SmiEntryPoint() function is copied to another
> memory location and run. But, _ SmiEntryPoint() calls the external C function
> CpuSmmDebugEntry(). nasm compiler generates the function call with the
> relative address. After _SmiEntryPoint() function is copied and run in the new
> address, its external function call will not work. To fix it, I add jmp 
> instruction
> to the original address, then process function all and works.
> 
> mov rax, strict qword 0 ;   mov rax, _SmiHandler
> _SmiHandlerAbsAddr:
> jmp rax
> 
> ...
> mov rcx, rbx
> callASM_PFX(CpuSmmDebugEntry)
> 
> Thanks
> Liming
> >-Original Message-
> >From: Yao, Jiewen
> >Sent: Wednesday, September 12, 2018 6:03 AM
> >To: Laszlo Ersek ; Gao, Liming
> ;
> >edk2-devel@lists.01.org
> >Cc: Dong, Eric 
> >Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >unnecessary jmp _SmiHandler
> >
> >HI
> >Would you please add info on what unit test has been done?
> >
> >Thank you
> >Yao Jiewen
> >
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Tuesday, September 11, 2018 11:06 PM
> >> To: Gao, Liming ; edk2-devel@lists.01.org
> >> Cc: Yao, Jiewen ; Dong, Eric
> 
> >> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> >> unnecessary jmp _SmiHandler
> >>
> >> On 09/10/18 10:20, Liming Gao wrote:
> >> > This change is wrong introduced by
> >> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
> >> > It is not required. So, revert it.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Liming Gao 
> >> > Cc: Jiewen Yao 
> >> > ---
> >> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
> >> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> > index 315d0f8..7b1b3ca 100644
> >> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> >> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
> >> >  mov gs, eax
> >> >  mov ax, [rbx + DSC_SS]
> >> >  mov ss, eax
> >> > -mov rax, strict qword 0 ;   mov rax,
> >> _SmiHandler
> >> > -_SmiHandlerAbsAddr:
> >> > -jmp rax
> >> > +
> >> > +;   jmp _SmiHandler ; instruction is not
> needed
> >> >
> >> >  _SmiHandler:
> >> >  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
> >> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
> >> >  learax, [ASM_PFX(gSmiHandlerIdtr)]
> >> >  learcx, [SmiHandlerIdtrAbsAddr]
> >> >  movqword [rcx - 8], rax
> >> > -
> >> > -learax, [_SmiHandler]
> >> > -learcx, [_SmiHandlerAbsAddr]
> >> > -movqword [rcx - 8], rax
> >> >  ret
> >> >
> >>
> >> Please remember to CC package maintainers / reviewers directly.
> >>
> >> The patch makes sense to me, and indeed it restores the original code.
> >>
> >> Reviewed-by: Laszlo Ersek 
> >>
> >> Can you perhaps add another sentence to the commit message, before
> you
> >> push the patch, such as "the original code already uses RIP-relative
> >> addressing"?
> >>
> >> Thanks
> >> Laszlo
> >> ___
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Gao, Liming]

Jiewen:
  After do more verification, I recall this change. Current code is really 
required. Without it, OVMF SMM can't boot. So, below code can't be removed.
  The reason is that nasm _SmiEntryPoint() function is copied to another memory 
location and run. But, _ SmiEntryPoint() calls the external C function 
CpuSmmDebugEntry(). nasm compiler generates the function call with the relative 
address. After _SmiEntryPoint() function is copied and run in the new address, 
its external function call will not work. To fix it, I add jmp instruction to 
the original address, then process function all and works. 
  
mov rax, strict qword 0 ;   mov rax, _SmiHandler
_SmiHandlerAbsAddr:
jmp rax

...
mov rcx, rbx
callASM_PFX(CpuSmmDebugEntry)

Thanks
Liming
>-Original Message-
>From: Yao, Jiewen
>Sent: Wednesday, September 12, 2018 6:03 AM
>To: Laszlo Ersek ; Gao, Liming ;
>edk2-devel@lists.01.org
>Cc: Dong, Eric 
>Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>unnecessary jmp _SmiHandler
>
>HI
>Would you please add info on what unit test has been done?
>
>Thank you
>Yao Jiewen
>
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Tuesday, September 11, 2018 11:06 PM
>> To: Gao, Liming ; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen ; Dong, Eric 
>> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> unnecessary jmp _SmiHandler
>>
>> On 09/10/18 10:20, Liming Gao wrote:
>> > This change is wrong introduced by
>> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
>> > It is not required. So, revert it.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Liming Gao 
>> > Cc: Jiewen Yao 
>> > ---
>> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
>> >  1 file changed, 2 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> > index 315d0f8..7b1b3ca 100644
>> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
>> >  mov gs, eax
>> >  mov ax, [rbx + DSC_SS]
>> >  mov ss, eax
>> > -mov rax, strict qword 0 ;   mov rax,
>> _SmiHandler
>> > -_SmiHandlerAbsAddr:
>> > -jmp rax
>> > +
>> > +;   jmp _SmiHandler ; instruction is not needed
>> >
>> >  _SmiHandler:
>> >  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
>> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>> >  learax, [ASM_PFX(gSmiHandlerIdtr)]
>> >  learcx, [SmiHandlerIdtrAbsAddr]
>> >  movqword [rcx - 8], rax
>> > -
>> > -learax, [_SmiHandler]
>> > -learcx, [_SmiHandlerAbsAddr]
>> > -movqword [rcx - 8], rax
>> >  ret
>> >
>>
>> Please remember to CC package maintainers / reviewers directly.
>>
>> The patch makes sense to me, and indeed it restores the original code.
>>
>> Reviewed-by: Laszlo Ersek 
>>
>> Can you perhaps add another sentence to the commit message, before you
>> push the patch, such as "the original code already uses RIP-relative
>> addressing"?
>>
>> Thanks
>> Laszlo
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

[Ni, Ruiyu]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, September 12, 2018 5:03 AM
> To: Ni, Ruiyu 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
> 
> On 11 September 2018 at 11:13, Ni, Ruiyu  wrote:
> > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
> >>
> >> On 11 September 2018 at 07:16, Jian J Wang  wrote:
> >>>
> >>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
> >>>
> >>> Since the stack memory is allocated as EfiBootServicesData, its NX
> >>> protection can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy.
> >>> To avoid confusing in setting related PCDs, PcdSetNxForStack will be
> >>> expired. Instead, If
> >>> BIT4
> >>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit
> >>> in page table entries mapping the stack memory.
> >>>
> >>
> >> I disagree. This removes the possibility to map EfiBootServicesData
> >> as executable while still mapping the stack NX. As we all know, an
> >> executable stack is in a class of its own when it comes to
> >> exploitability, and should *never* be mapped executable unless in
> >> highly exceptional cases. Mapping all EfiBootServicesData as
> >> non-executable may cause backward compatibility problems.
> >
> > Ard,
> > Are you saying you want the capability of setting certain range of BS
> > data as executable? Why does ARM need such capability?
> >
> 
> No, I am saying that mapping all BS data executable should be a separate
> decision from mapping the stack executable: if your platform cannot support 
> the
> former (for historical reasons) you will likely still want the latter.

Let me try to understand the specific problem in ARM64:
ARM64 uses 64KB page size to support 2^52 memory space. With 4KB page size,
it can only support 2^48 memory space.
But due to the DXE core AllocatePages() implementation, the hard-code 4KB 
granularity
(defined by UEFI spec) causes the page table protection for BS_DATA/BS_CODE is 
impossible.
So ARM64 chooses to disable the BS_DATA/BS_CODE protection, but only enable
the stack protection.
Correct?
If so, is changing spec to allow page-size platform configurable a better 
option?

> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Wang, Jian J]

Laszlo,

Thanks for the comments.


(1)Agree. It’ll be updated in v2.

(2)DEBUG_ERROR level won’t print word “ERROR” on console. I think an “ERROR”

word in log should get the attention more easily.

(3)How about log both of them? GUID value may be more friendly to log 
parser but
a GUID name is more friendly to person.

(4)Good idea. I’ll add it in v2.

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 10:01 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Zeng, Star ; You, Benjamin ; 
Dong, Eric 
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/10/18 05:22, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
>
> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if
> PcdAcpiS3Enable is set to TRUE. Current code in CpuS3.c doesn't
> embody this strong binding between them. An error message and
> ASSERT is added by this patch to warn platform developer about
> it.
>
> Cc: Star Zeng mailto:star.z...@intel.com>>
> Cc: Benjamin You mailto:benjamin@intel.com>>
> Cc: Eric Dong mailto:eric.d...@intel.com>>
> Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> mailto:jian.j.w...@intel.com>>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..f371667c44 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>  if (sizeof (UINTN) == sizeof (UINT32)) {
>SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>  }
> +  } else {
> +DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 
> resume doesn't exist!\n"));
> +ASSERT (FALSE);
>}
>
>//
>

I have some superficial comments for this patch.

(1) in case the "if" has an "else" branch, I think it's better style to
use "==" in the condition than "!=". To me,

  if (GuidHob != NULL) {
//
// do a bunch of stuff
//
  } else {
//
// error
//
  }

is more difficult to read than:

  if (GuidHob == NULL) {
//
// error
//
  } else {
//
// do a bunch of stuff
//
  }

in particular if the "bunch of stuff" includes nested "if" statements.


(2) I think the error message could be improved. I'd suggest removing
the word "ERROR", since DEBUG_ERROR already sets the error level / mask.

(3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
textually, as a string -- in edk2 we generally log GUIDs by value, and
log parsers / translators usually look for GUID values. Thus I suggest
using %g with 

(4) Please consider logging the module name and/or the function name
too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
"gEfiCallerBaseName" is usually only helpful with libraries (because
they can be linked into multiple drivers), but __FUNCTION__ is more
frequently helpful.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Wang, Jian J]

Laszlo,

Thanks for the comment. I think it’ll ok to add it in a separate patch.

Just a little confuse about “a separate patch”. Does it mean a separate patch 
file
in the same patch series or a separate patch which needs a separate BZ tracker?

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 9:52 PM
To: Zeng, Star ; Wang, Jian J ; 
edk2-devel@lists.01.org
Cc: You, Benjamin ; Dong, Eric 
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/10/18 07:07, Zeng, Star wrote:
> I agree to add the ASSERT, but even with the ASSERT, I still suggest moving
> //
> // Patch SmmS3ResumeState->SmmS3Cr3
> //
> InitSmmS3Cr3 ();
>
> into
>   GuidHob = GetFirstGuidHob ();
>   if (GuidHob != NULL) {
> ...
>   }
>
> With that, Reviewed-by: Star Zeng 
> mailto:star.z...@intel.com>>

I think that's a valid idea, but shouldn't it be done in a separate
patch? One patch for the assert, and another moving InitSmmS3Cr3() under
the right condition. Does that sound OK?

Thanks
Laszlo


>
>
> Thanks,
> Star
> -Original Message-
> From: Wang, Jian J
> Sent: Monday, September 10, 2018 11:22 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star mailto:star.z...@intel.com>>; You, 
> Benjamin mailto:benjamin@intel.com>>; Dong, Eric 
> mailto:eric.d...@intel.com>>; Laszlo Ersek 
> mailto:ler...@redhat.com>>
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
>
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
>
> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if PcdAcpiS3Enable 
> is set to TRUE. Current code in CpuS3.c doesn't embody this strong binding 
> between them. An error message and ASSERT is added by this patch to warn 
> platform developer about it.
>
> Cc: Star Zeng mailto:star.z...@intel.com>>
> Cc: Benjamin You mailto:benjamin@intel.com>>
> Cc: Eric Dong mailto:eric.d...@intel.com>>
> Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> mailto:jian.j.w...@intel.com>>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..f371667c44 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>  if (sizeof (UINTN) == sizeof (UINT32)) {
>SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>  }
> +  } else {
> +DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 
> resume doesn't exist!\n"));
> +ASSERT (FALSE);
>}
>
>//
> --
> 2.16.2.windows.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: suppress compiler complaining

[Wang, Jian J]

Laszlo,

Thanks for the comments.


(1)Sure. My fault. I thought it’s just very small change for compiler 
warning.

(2)From language point view, it’s a valid warning. But from code logic, 
it’s invalid.

(3)Agree. It’ll be dropped.

(4)Agree. I’ll change it.


Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 10:56 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Wu, Hao A ; Bi, Dandan ; Dong, 
Eric 
Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: suppress compiler complaining

Jian,

On 09/11/18 06:47, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1166
>
> Cc: Dandan Bi mailto:dandan...@intel.com>>
> Cc: Hao A Wu mailto:hao.a...@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> mailto:jian.j.w...@intel.com>>
> ---
>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 23 ---
>  1 file changed, 16 insertions(+), 7 deletions(-)

(1) Please remember to CC the package maintainers / reviewers on
patches. "Maintainers.txt" lists Eric (M) and myself (R) for UefiCpuPkg.
It's OK to CC other people as well, of course.

(2) Bug 1166 mentions "warning C4701: potentially uninitialized local
variable 'StackBase' used".

If that warning is invalid (= the variable can never be read
unassigned), then we have some suggested language for that; please see
.

Furthermore:

>
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index bcb942a8e5..a63421a1af 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -517,7 +517,7 @@ GetStackBase (
>IN OUT VOID *Buffer
>)
>  {
> -  EFI_PHYSICAL_ADDRESSStackBase;
> +  volatile EFI_PHYSICAL_ADDRESS   StackBase;

(3) "volatile" seems unrelated; I suggest dropping it.

(Especially without the comment mentioned in TianoCore#607, "volatile"
is totally unjustified and confusing.)

>
>StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)
>StackBase += BASE_4KB;
> @@ -554,6 +554,8 @@ SetupStackGuardPage (
>MpInitLibGetNumberOfProcessors(, NULL);
>MpInitLibWhoAmI ();
>for (Index = 0; Index < NumberOfProcessors; ++Index) {
> +StackBase = 0;
> +
>  if (Index == Bsp) {
>Hob.Raw = GetHobList ();
>while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, 
> Hob.Raw)) != NULL) {
> @@ -570,12 +572,19 @@ SetupStackGuardPage (
>//
>MpInitLibStartupThisAP(GetStackBase, Index, NULL, 0, (VOID 
> *), NULL);
>  }
> -//
> -// Set Guard page at stack base address.
> -//
> -ConvertMemoryPageAttributes(StackBase, EFI_PAGE_SIZE, 0);
> -DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
> -(UINT64)StackBase, (UINT64)Index));
> +
> +if (StackBase == 0) {
> +  DEBUG ((DEBUG_ERROR, "Stack base address was not found for 
> [cpu%lu]!\n",
> +  (UINT64)Index));
> +  ASSERT(StackBase != 0);

(4) On the other hand, if it *can* happen in practice that the stack
base is not found (and in that case, we should halt), then:

* the subject line is wrong, because the compiler warning is *valid*,
and we don't suppress it, but fix the issue caught by the compiler;

* we must not proceed in a RELEASE build either, therefore an ASSERT is
insufficient. A CpuDeadLoop() is necessary.

(Again, this only applies if StackBase may be zero here by design.)

Thanks
Laszlo


> +} else {
> +  //
> +  // Set Guard page at stack base address.
> +  //
> +  ConvertMemoryPageAttributes(StackBase, EFI_PAGE_SIZE, 0);
> +  DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
> +  (UINT64)StackBase, (UINT64)Index));
> +}
>}
>
>//
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Yao, Jiewen]

HI
Would you please add info on what unit test has been done?

Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, September 11, 2018 11:06 PM
> To: Gao, Liming ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> unnecessary jmp _SmiHandler
> 
> On 09/10/18 10:20, Liming Gao wrote:
> > This change is wrong introduced by
> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
> > It is not required. So, revert it.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Liming Gao 
> > Cc: Jiewen Yao 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> > index 315d0f8..7b1b3ca 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
> >  mov gs, eax
> >  mov ax, [rbx + DSC_SS]
> >  mov ss, eax
> > -mov rax, strict qword 0 ;   mov rax,
> _SmiHandler
> > -_SmiHandlerAbsAddr:
> > -jmp rax
> > +
> > +;   jmp _SmiHandler ; instruction is not needed
> >
> >  _SmiHandler:
> >  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
> >  learax, [ASM_PFX(gSmiHandlerIdtr)]
> >  learcx, [SmiHandlerIdtrAbsAddr]
> >  movqword [rcx - 8], rax
> > -
> > -learax, [_SmiHandler]
> > -learcx, [_SmiHandlerAbsAddr]
> > -movqword [rcx - 8], rax
> >  ret
> >
> 
> Please remember to CC package maintainers / reviewers directly.
> 
> The patch makes sense to me, and indeed it restores the original code.
> 
> Reviewed-by: Laszlo Ersek 
> 
> Can you perhaps add another sentence to the commit message, before you
> push the patch, such as "the original code already uses RIP-relative
> addressing"?
> 
> Thanks
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

[Ard Biesheuvel]

On 11 September 2018 at 11:13, Ni, Ruiyu  wrote:
> On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
>>
>> On 11 September 2018 at 07:16, Jian J Wang  wrote:
>>>
>>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>>>
>>> Since the stack memory is allocated as EfiBootServicesData, its NX
>>> protection
>>> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid
>>> confusing
>>> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If
>>> BIT4
>>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in
>>> page
>>> table entries mapping the stack memory.
>>>
>>
>> I disagree. This removes the possibility to map EfiBootServicesData as
>> executable while still mapping the stack NX. As we all know, an
>> executable stack is in a class of its own when it comes to
>> exploitability, and should *never* be mapped executable unless in
>> highly exceptional cases. Mapping all EfiBootServicesData as
>> non-executable may cause backward compatibility problems.
>
> Ard,
> Are you saying you want the capability of setting certain range of BS data
> as executable? Why does ARM need such capability?
>

No, I am saying that mapping all BS data executable should be a
separate decision from mapping the stack executable: if your platform
cannot support the former (for historical reasons) you will likely
still want the latter.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.

[Duran, Leo]

> -Original Message-
> From: Laszlo Ersek 
> Sent: Tuesday, September 11, 2018 1:50 PM
> To: Duran, Leo ; edk2-devel@lists.01.org
> Cc: Eric Dong ; Ruiyu Ni 
> Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
> prior to MTRR change.
> 
> On 09/11/18 17:41, Leo Duran wrote:
> > The default behavior is to disable MTRRs prior to an MTRR change.
> > However, on SMT platforms with shared CPU resources it may be
> > desirable to skip the default behavior, and leave the current state of the
> Enable bit.
> >
> > Cc: Eric Dong 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Leo Duran 
> > ---
> >  UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++---
> >  UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
> >  UefiCpuPkg/UefiCpuPkg.dec  |  7 +++
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > index dfce9a9..baf9a0f 100644
> > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > @@ -6,6 +6,8 @@
> >  except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting 
> > to
> APs.
> >
> >Copyright (c) 2008 - 2018, Intel Corporation. All rights
> > reserved.
> > +  Copyright (c) 2018, AMD Inc. All rights reserved.
> > +
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions of the
> BSD License
> >which accompanies this distribution.  The full text of the license
> > may be found at @@ -310,9 +312,11 @@ MtrrLibPreMtrrChange (
> >//
> >// Disable MTRRs
> >//
> > -  DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> > -  DefType.Bits.E = 0;
> > -  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
> > +  if (!PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChange)) {
> > +DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> > +DefType.Bits.E = 0;
> > +AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);  }
> >  }
> >
> >  /**
> > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> > index a97cc61..06f33e8 100644
> > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> > @@ -2,6 +2,8 @@
> >  #  MTRR library provides APIs for MTRR operation.
> >  #
> >  #  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> > +#  Copyright (c) 2018, AMD Inc. All rights reserved.
> > +#
> >  #  This program and the accompanying materials
> >  #  are licensed and made available under the terms and conditions of the
> BSD License
> >  #  which accompanies this distribution.  The full text of the license may 
> > be
> found at
> > @@ -43,4 +45,5 @@
> >
> >  [Pcd]
> >gUefiCpuPkgTokenSpaceGuid.PcdCpuNumberOfReservedVariableMtrrs
> ## SOMETIMES_CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange
> ## CONSUMES
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > index 69d777a..64ec763 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > @@ -2,6 +2,7 @@
> >  # This Package provides UEFI compatible CPU modules and libraries.
> >  #
> >  # Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
> > +# Copyright (c) 2018, AMD Inc. All rights reserved.
> >  #
> >  # This program and the accompanying materials are licensed and made
> available under
> >  # the terms and conditions of the BSD License which accompanies this
> distribution.
> > @@ -273,6 +274,12 @@
> ># @Prompt Current boot is a power-on reset.
> >
> gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x
> 001B
> >
> > +  ## Indicates desired behavior for disabling MTRRs prior to MTRR
> change.
> > +  #   TRUE  - Skip disabling MTRRs prior to MTRR change.
> > +  #   FALSE - Disable MTRRs prior to MTRR change.
> > +  # @Prompt Desired behavior for disabling MTRRs prior to MTRR change.
> > +
> gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange|FALSE
> |BOOLEAN|0x001E
> > +
> >  [PcdsDynamic, PcdsDynamicEx]
> >## Contains the pointer to a CPU S3 data buffer of structure
> ACPI_CPU_DATA.
> ># @Prompt The pointer to a CPU S3 data buffer.
> >
> 
> Recently, Ray has written several & significant patches for MtrrLib; I'm
> adding him.
> 
> I don't understand the motivation behind this patch. As far as I
> remember (which is admittedly "quite vaguely"), the SDM requires all
> logical processors to program their MTRRs identically in parallel. That
> is, there should be a start line where all the CPUs wait for each other,
> then they all set up their MTRRs, then they all wait until they all
> finish, then they all go their merry ways. AIUI the CPU MP PPI and
> protocol implement this already. I don't understand in what situation
> you'd have one thread of a core manipulating MTRR, with the sibling

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.

[Laszlo Ersek]

On 09/11/18 17:41, Leo Duran wrote:
> The default behavior is to disable MTRRs prior to an MTRR change.
> However, on SMT platforms with shared CPU resources it may be desirable to
> skip the default behavior, and leave the current state of the Enable bit.
> 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leo Duran 
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
>  UefiCpuPkg/UefiCpuPkg.dec  |  7 +++
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c 
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index dfce9a9..baf9a0f 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -6,6 +6,8 @@
>  except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting to 
> APs.
>  
>Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> +  Copyright (c) 2018, AMD Inc. All rights reserved.
> +
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
>which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -310,9 +312,11 @@ MtrrLibPreMtrrChange (
>//
>// Disable MTRRs
>//
> -  DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> -  DefType.Bits.E = 0;
> -  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
> +  if (!PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChange)) {
> +DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> +DefType.Bits.E = 0;
> +AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
> +  }
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf 
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> index a97cc61..06f33e8 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> @@ -2,6 +2,8 @@
>  #  MTRR library provides APIs for MTRR operation.
>  #
>  #  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +#  Copyright (c) 2018, AMD Inc. All rights reserved.
> +#
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD 
> License
>  #  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -43,4 +45,5 @@
>  
>  [Pcd]
>gUefiCpuPkgTokenSpaceGuid.PcdCpuNumberOfReservedVariableMtrrs   ## 
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange## CONSUMES
>  
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 69d777a..64ec763 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -2,6 +2,7 @@
>  # This Package provides UEFI compatible CPU modules and libraries.
>  #
>  # Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2018, AMD Inc. All rights reserved.
>  #
>  # This program and the accompanying materials are licensed and made 
> available under
>  # the terms and conditions of the BSD License which accompanies this 
> distribution.
> @@ -273,6 +274,12 @@
># @Prompt Current boot is a power-on reset.
>gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x001B
>  
> +  ## Indicates desired behavior for disabling MTRRs prior to MTRR 
> change.
> +  #   TRUE  - Skip disabling MTRRs prior to MTRR change.
> +  #   FALSE - Disable MTRRs prior to MTRR change.
> +  # @Prompt Desired behavior for disabling MTRRs prior to MTRR change.
> +  
> gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange|FALSE|BOOLEAN|0x001E
> +
>  [PcdsDynamic, PcdsDynamicEx]
>## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
># @Prompt The pointer to a CPU S3 data buffer.
> 

Recently, Ray has written several & significant patches for MtrrLib; I'm
adding him.

I don't understand the motivation behind this patch. As far as I
remember (which is admittedly "quite vaguely"), the SDM requires all
logical processors to program their MTRRs identically in parallel. That
is, there should be a start line where all the CPUs wait for each other,
then they all set up their MTRRs, then they all wait until they all
finish, then they all go their merry ways. AIUI the CPU MP PPI and
protocol implement this already. I don't understand in what situation
you'd have one thread of a core manipulating MTRR, with the sibling
thread *not* manipulating MTRR (i.e., doing something else). That
doesn't seem to match what the SDM dictates (or, well, what my memories
of the SDM are :) ).

I see that the default behavior doesn't change, and I'm not against the
patch; I just suspect that, for introducing a new PCD, more concrete /
practical justification could be needed.

More questions:

- Don't you need a similar (symmetric) change in
MtrrLibPostMtrrChange()? If not, why not? Can you 

Re: [edk2] [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation

[Laszlo Ersek]

On 09/07/18 02:07, Robinson, Herbie wrote:
> This is a fix for a double cluster allocation when the disk is full.  The 
> double allocation happens because FatGrowEof calls FatAllocateCluster without 
> immediately marking the each returned cluster as allocated.  The fix is to 
> move the FatSetFatEntry call inside the loop.  I've also include some 
> improvements to the sanity checks that I added while tracking this down.  
> They are optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by:Herbie Robinson 

CC'ing Ray (see "Maintainers.txt").

Thanks
Laszlo

> ---
> diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c 
> b/FatPkg/EnhancedFatDxe/FileSpace.c
> index 1254cd6..e17d3b6 100644
> --- a/FatPkg/EnhancedFatDxe/FileSpace.c
> +++ b/FatPkg/EnhancedFatDxe/FileSpace.c
> @@ -467,7 +467,7 @@ FatGrowEof (
>ClusterCount  = 0;
> 
>while (!FAT_END_OF_FAT_CHAIN (Cluster)) {
> -if (Cluster == FAT_CLUSTER_FREE || Cluster >= FAT_CLUSTER_SPECIAL) {
> +if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
> 
>DEBUG (
>  (EFI_D_INIT | EFI_D_ERROR,
> @@ -509,6 +509,11 @@ FatGrowEof (
>  goto Done;
>}
> 
> +  if (NewCluster < FAT_MIN_CLUSTER || NewCluster > Volume->MaxCluster + 
> 1) {
> +Status = EFI_VOLUME_CORRUPTED;
> +goto Done;
> +  }
> +
>if (LastCluster != 0) {
>  FatSetFatEntry (Volume, LastCluster, NewCluster);
>} else {
> @@ -518,12 +523,21 @@ FatGrowEof (
> 
>LastCluster = NewCluster;
>CurSize += 1;
> +
> +  //
> +  // Terminate the cluster list
> +  //
> +  // Note that we must do this EVERY time we allocate a cluster, because
> +  // FatAllocateCluster scans the FAT looking for a free cluster and
> +  // "LastCluster" is no longer free!  Usually, FatAllocateCluster will
> +  // start looking with the cluster after "LastCluster"; however, when
> +  // there is only one free cluster left, it will find "LastCluster"
> +  // a second time.  There are other, less predictable scenarios
> +  // where this could happen, as well.
> +  //
> +  FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> +  OFile->FileLastCluster = LastCluster;
>  }
> -//
> -// Terminate the cluster list
> -//
> -FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> -OFile->FileLastCluster = LastCluster;
>}
> 
>OFile->FileSize = (UINTN) NewSizeInBytes;
> @@ -603,7 +617,7 @@ FatOFilePosition (
>Cluster = FatGetFatEntry (Volume, Cluster);
>  }
> 
> -if (Cluster < FAT_MIN_CLUSTER) {
> +if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
>return EFI_VOLUME_CORRUPTED;
>  }
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack

[Laszlo Ersek]

On 09/11/18 07:16, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since PcdSetNxForStack is expired, remove related config code.
> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
> There's no need to add it in code to replace PcdSetNxForStack.
>
> Cc: Laszlo Ersek 
> Cc: Star Zeng 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Ruiyu Ni 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  OvmfPkg/PlatformPei/Platform.c  | 1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126..6627d236e0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>)
>  {
>UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
> -  UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
>  }
>
>  VOID
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 9c5ad9961c..ef5dcb7693 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,7 +96,6 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>

I disagree with this change. I explained my reasons in
, but for the
sake of discussion, I'll state the same here:

> Ard's got a point here. Just because one PCD implies another, the
> reverse is not necessarily true.
>
> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
> then we had to make it conditional, due to old GRUB variants breaking
> with a non-executable stack. (Please refer to commit d20b06a3afdf,
> "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15).
> Therefore we now default to FALSE, and let the user set it to TRUE on
> the QEMU command line.
>
> In addition, we intentionally inherit a zero
> PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec".
>
> Both of the above configurations satisfy the requirement
>
>   ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> (1 << EfiBootServicesData)) == 0 ||
>PcdGetBool (PcdSetNxForStack))
>
> i.e. the requirement that "NX for BS data" imply "NX for stack".
>
> If you remove the standalone knob for PcdSetNxForStack, then the
> default behavior of OVMF will continue to work, however the command
> line option, for setting PcdSetNxForStack *only*, will break.

I'd like to add another comment just here (not mentioned in the BZ):

To my understanding, the Heap Guard is a debug feature [1]. Over time,
I've reviewed and regression-tested all the Heap Guard patches that have
crossed my path with the understanding that "this is not enabled by
default in OVMF". With those points in mind, I certainly don't intend to
enable the Heap Guard as a FixedPCD -- which is the only way it can be
enabled -- in the OVMF DSC files anytime soon.

On the other hand, the user should set the stack NX preferably at all
times -- as I wrote above, that was our original idea for the OVMF
default as well, until we were forced to revert it for compatibility's
sake, and to expose the knob to the end-user instead.

With this patch series, the configuration that's currently deemed the
best compromise for OVMF (Heap Guard off, stack NX) would be lost.

[1] 
http://mid.mail-archive.com/D827630B58408649ACB04F44C510003624DDC8B3@SHSMSX103.ccr.corp.intel.com

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.

[Leo Duran]

The default behavior is to disable MTRRs prior to an MTRR change.
However, on SMT platforms with shared CPU resources it may be desirable to
skip the default behavior, and leave the current state of the Enable bit.

Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leo Duran 
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
 UefiCpuPkg/UefiCpuPkg.dec  |  7 +++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c 
b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index dfce9a9..baf9a0f 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -6,6 +6,8 @@
 except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting to 
APs.
 
   Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
+  Copyright (c) 2018, AMD Inc. All rights reserved.
+
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -310,9 +312,11 @@ MtrrLibPreMtrrChange (
   //
   // Disable MTRRs
   //
-  DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
-  DefType.Bits.E = 0;
-  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
+  if (!PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChange)) {
+DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
+DefType.Bits.E = 0;
+AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
+  }
 }
 
 /**
diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf 
b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
index a97cc61..06f33e8 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
@@ -2,6 +2,8 @@
 #  MTRR library provides APIs for MTRR operation.
 #
 #  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
+#  Copyright (c) 2018, AMD Inc. All rights reserved.
+#
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD 
License
 #  which accompanies this distribution.  The full text of the license may be 
found at
@@ -43,4 +45,5 @@
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuNumberOfReservedVariableMtrrs   ## 
SOMETIMES_CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange## CONSUMES
 
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 69d777a..64ec763 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -2,6 +2,7 @@
 # This Package provides UEFI compatible CPU modules and libraries.
 #
 # Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2018, AMD Inc. All rights reserved.
 #
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License which accompanies this 
distribution.
@@ -273,6 +274,12 @@
   # @Prompt Current boot is a power-on reset.
   gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x001B
 
+  ## Indicates desired behavior for disabling MTRRs prior to MTRR 
change.
+  #   TRUE  - Skip disabling MTRRs prior to MTRR change.
+  #   FALSE - Disable MTRRs prior to MTRR change.
+  # @Prompt Desired behavior for disabling MTRRs prior to MTRR change.
+  
gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange|FALSE|BOOLEAN|0x001E
+
 [PcdsDynamic, PcdsDynamicEx]
   ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
   # @Prompt The pointer to a CPU S3 data buffer.
-- 
2.7.4

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] Add flag to skip disabling MTRRs

[Leo Duran]

This patch adds a flag that will allow us to skip disabling MTRRs on SMT
platforms where the MTRR Enable bit is shared across threads in a CPU core.

The default behavior is unchanged, so existing implementations are not
affected by this patch.

Leo Duran (1):
  UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR
change.

 UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
 UefiCpuPkg/UefiCpuPkg.dec  |  7 +++
 3 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.7.4

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

[Laszlo Ersek]

On 09/04/18 15:43, Stephen Benjamin wrote:
> On Tue, Sep 4, 2018 at 7:02 AM Laszlo Ersek  wrote:
>>
>> On 09/04/18 09:17, Jiaxin Wu wrote:
>>> This patch is to resolve the lock-up issue if the value of HTTP header
>>> is blank.  The issue is recorded @
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1102.
>>>
>>> Cc: Stephen Benjamin 
>>> Cc: Laszlo Ersek 
>>> Cc: Ye Ting 
>>> Cc: Fu Siyuan 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Wu Jiaxin 
>>> ---
>>>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
>>>  1 file changed, 44 insertions(+), 13 deletions(-)
>>
>> Thank you, Jiaxin, I'll let Stephen test this :)
>>
>> Stephen, can you please respond with your Tested-by, if the patch solves
>> the problem for you?
>>
>> Also, would you prefer a remote repo+branch that you could pull (over
>> applying this patch with "git-am") for testing? The edk2 project uses
>> CRLF source files, so "git-am" is a bit quirky.
> 
> No need, the patch applied fine. Thanks for the quick turnaround! It
> resolved my issue.
> 
> Tested-by: Stephen Benjamin 

Siyuan, Ting, can you guys please review Jiaxin's patch?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

[Laszlo Ersek]

On 09/10/18 10:20, Liming Gao wrote:
> This change is wrong introduced by e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
> It is not required. So, revert it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao 
> Cc: Jiewen Yao 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> index 315d0f8..7b1b3ca 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
>  mov gs, eax
>  mov ax, [rbx + DSC_SS]
>  mov ss, eax
> -mov rax, strict qword 0 ;   mov rax, _SmiHandler
> -_SmiHandlerAbsAddr:
> -jmp rax
> +
> +;   jmp _SmiHandler ; instruction is not needed
>  
>  _SmiHandler:
>  mov rbx, [rsp + 0x8] ; rcx <- CpuIndex
> @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>  learax, [ASM_PFX(gSmiHandlerIdtr)]
>  learcx, [SmiHandlerIdtrAbsAddr]
>  movqword [rcx - 8], rax
> -
> -learax, [_SmiHandler]
> -learcx, [_SmiHandlerAbsAddr]
> -movqword [rcx - 8], rax
>  ret
> 

Please remember to CC package maintainers / reviewers directly.

The patch makes sense to me, and indeed it restores the original code.

Reviewed-by: Laszlo Ersek 

Can you perhaps add another sentence to the commit message, before you
push the patch, such as "the original code already uses RIP-relative
addressing"?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: suppress compiler complaining

[Laszlo Ersek]

Jian,

On 09/11/18 06:47, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1166
> 
> Cc: Dandan Bi 
> Cc: Hao A Wu 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 23 ---
>  1 file changed, 16 insertions(+), 7 deletions(-)

(1) Please remember to CC the package maintainers / reviewers on
patches. "Maintainers.txt" lists Eric (M) and myself (R) for UefiCpuPkg.
It's OK to CC other people as well, of course.

(2) Bug 1166 mentions "warning C4701: potentially uninitialized local
variable 'StackBase' used".

If that warning is invalid (= the variable can never be read
unassigned), then we have some suggested language for that; please see
.

Furthermore:

> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index bcb942a8e5..a63421a1af 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -517,7 +517,7 @@ GetStackBase (
>IN OUT VOID *Buffer
>)
>  {
> -  EFI_PHYSICAL_ADDRESSStackBase;
> +  volatile EFI_PHYSICAL_ADDRESS   StackBase;

(3) "volatile" seems unrelated; I suggest dropping it.

(Especially without the comment mentioned in TianoCore#607, "volatile"
is totally unjustified and confusing.)

>  
>StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)
>StackBase += BASE_4KB;
> @@ -554,6 +554,8 @@ SetupStackGuardPage (
>MpInitLibGetNumberOfProcessors(, NULL);
>MpInitLibWhoAmI ();
>for (Index = 0; Index < NumberOfProcessors; ++Index) {
> +StackBase = 0;
> +
>  if (Index == Bsp) {
>Hob.Raw = GetHobList ();
>while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, 
> Hob.Raw)) != NULL) {
> @@ -570,12 +572,19 @@ SetupStackGuardPage (
>//
>MpInitLibStartupThisAP(GetStackBase, Index, NULL, 0, (VOID 
> *), NULL);
>  }
> -//
> -// Set Guard page at stack base address.
> -//
> -ConvertMemoryPageAttributes(StackBase, EFI_PAGE_SIZE, 0);
> -DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
> -(UINT64)StackBase, (UINT64)Index));
> +
> +if (StackBase == 0) {
> +  DEBUG ((DEBUG_ERROR, "Stack base address was not found for 
> [cpu%lu]!\n",
> +  (UINT64)Index));
> +  ASSERT(StackBase != 0);

(4) On the other hand, if it *can* happen in practice that the stack
base is not found (and in that case, we should halt), then:

* the subject line is wrong, because the compiler warning is *valid*,
and we don't suppress it, but fix the issue caught by the compiler;

* we must not proceed in a RELEASE build either, therefore an ASSERT is
insufficient. A CpuDeadLoop() is necessary.

(Again, this only applies if StackBase may be zero here by design.)

Thanks
Laszlo


> +} else {
> +  //
> +  // Set Guard page at stack base address.
> +  //
> +  ConvertMemoryPageAttributes(StackBase, EFI_PAGE_SIZE, 0);
> +  DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
> +  (UINT64)StackBase, (UINT64)Index));
> +}
>}
>  
>//
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Laszlo Ersek]

On 09/10/18 05:22, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
> 
> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if
> PcdAcpiS3Enable is set to TRUE. Current code in CpuS3.c doesn't
> embody this strong binding between them. An error message and
> ASSERT is added by this patch to warn platform developer about
> it.
> 
> Cc: Star Zeng 
> Cc: Benjamin You 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..f371667c44 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>  if (sizeof (UINTN) == sizeof (UINT32)) {
>SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>  }
> +  } else {
> +DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 
> resume doesn't exist!\n"));
> +ASSERT (FALSE);
>}
>  
>//
> 

I have some superficial comments for this patch.

(1) in case the "if" has an "else" branch, I think it's better style to
use "==" in the condition than "!=". To me,

  if (GuidHob != NULL) {
//
// do a bunch of stuff
//
  } else {
//
// error
//
  }

is more difficult to read than:

  if (GuidHob == NULL) {
//
// error
//
  } else {
//
// do a bunch of stuff
//
  }

in particular if the "bunch of stuff" includes nested "if" statements.


(2) I think the error message could be improved. I'd suggest removing
the word "ERROR", since DEBUG_ERROR already sets the error level / mask.

(3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
textually, as a string -- in edk2 we generally log GUIDs by value, and
log parsers / translators usually look for GUID values. Thus I suggest
using %g with 

(4) Please consider logging the module name and/or the function name
too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
"gEfiCallerBaseName" is usually only helpful with libraries (because
they can be linked into multiple drivers), but __FUNCTION__ is more
frequently helpful.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Laszlo Ersek]

On 09/10/18 07:07, Zeng, Star wrote:
> I agree to add the ASSERT, but even with the ASSERT, I still suggest moving 
> //
> // Patch SmmS3ResumeState->SmmS3Cr3
> //
> InitSmmS3Cr3 ();
> 
> into
>   GuidHob = GetFirstGuidHob ();
>   if (GuidHob != NULL) {
> ...
>   }
> 
> With that, Reviewed-by: Star Zeng 

I think that's a valid idea, but shouldn't it be done in a separate
patch? One patch for the assert, and another moving InitSmmS3Cr3() under
the right condition. Does that sound OK?

Thanks
Laszlo


> 
> 
> Thanks,
> Star
> -Original Message-
> From: Wang, Jian J 
> Sent: Monday, September 10, 2018 11:22 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; You, Benjamin ; 
> Dong, Eric ; Laszlo Ersek 
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
> 
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
> 
> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if PcdAcpiS3Enable 
> is set to TRUE. Current code in CpuS3.c doesn't embody this strong binding 
> between them. An error message and ASSERT is added by this patch to warn 
> platform developer about it.
> 
> Cc: Star Zeng 
> Cc: Benjamin You 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..f371667c44 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>  if (sizeof (UINTN) == sizeof (UINT32)) {
>SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>  }
> +  } else {
> +DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 
> resume doesn't exist!\n"));
> +ASSERT (FALSE);
>}
>  
>//
> --
> 2.16.2.windows.1
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek]

On 09/07/18 19:01, Liran Alon wrote:
> 
> 
>> On 7 Sep 2018, at 11:44, Laszlo Ersek  wrote:
>>
>> (+Ard)
>>
>> On 09/06/18 21:08, Nikita Leshenko wrote:
>>> Hi,
>>>
>>> We ran into a bug in EDK2 relating to PCI-Express in PciBusDxe. Here's the 
>>> flow
>>> of the bug:
>>>
>>> 1. PciBusDxe/PciEnumeratorSupport.c: Function BarExisted probes a BAR. It 
>>> raises
>>>   TPL to TPL_HIGH_LEVEL to avoid timer interrupts while probing the BAR and
>>>   calls PciIo->Pci.Write.
>>> 2. BasePciExpressLib/PciExpressLib.c: The write reaches PciExpressWrite32, 
>>> which
>>>   calls GetPciExpressBaseAddress.
>>> 3. GetPciExpressBaseAddress retrieves the address from 
>>> PcdPciExpressBaseAddress.
>>> 4. Reading the PCD calls DxePcdGet64 -> GetWorker ->
>>>   EfiAcquireLock(), which is at TPL_NOTIFY level. This 
>>> crashes
>>>   the firmware because step 1 raised the TPL to TPL_HIGH_LEVEL.
>>>
>>> This doesn't happen when PcdPciExpressBaseAddress is fixed at build (because
>>> then the read is optimized to a static global variable), but when the PCD is
>>> dynamic PCI-Express is broken.
>>>
>>> Does anybody have a suggestion for fixing it?
>>>
>>> Options we thought about:
>>> - Change mPcdDatabaseLock.Tpl to TPL_HIGH_LEVEL
>>> - Don't use a PCD for the base address, put it in a static global variable 
>>> and
>>>  create functions to set and retrieve it.
>>
>> In the ArmVirtPkg platforms, we also set "PcdPciExpressBaseAddress"
>> dynamically. And, we implemented your second option above; see:
>>
>>  ArmVirtPkg/Library/BaseCachingPciExpressLib/
>>
>> Relevant commits:
>>
>> - ad3359eb43a9 ("ArmVirtualizationPkg: clone BasePciExpressLib, cache
>> PCIe config base", 2015-02-23)
>> - a06d0bb58eb9 ("ArmVirtPkg/BaseCachingPciExpressLib: depend on
>> PciPcdProducerLib", 2016-04-12)
>>
>> (In fact, commit ad3359eb43a9 documents the exact issue you report here.)
>>
>> Thanks
>> Laszlo
> 
> Thanks Lazlo. Weren’t aware of this solution in the ArmVirtPkg platforms.
> 
> However, I wonder why the solution was to clone the 
> MdePkg/Library/BasePciExpressLib rather than
> change the original library itself?
> 
> That is, what was the reason for not just adding a library constructor to 
> MdePkg/Library/BasePciExpressLib
> to cache PcdPciExpressBaseAddress in a global var? This seem to solve the 
> issues for all platforms.

There are two reasons, one related to project governance, and another
technical.

(1) The reason related to project management is that modifying core edk2
libraries and drivers (such that live in MdePkg and MdeModulePkg) is
usually difficult. The requirement that the code be correct and pass
technical review is the easy part; the hard part is that you can sell
your use case enough for your change to be accepted for MdePkg /
MdeModulePkg. I'm pretty unhappy about this state of affairs myself, but
it is what it is. (I do sympathize with the MdePkg / MdeModulePkg
maintainers as well, though! Their responsibility is big.) So, in many
cases, it is a lot easier to enable your platform by cloning a library
instance and modifying it. After all, that's what library *classes* were
invented for -- to have multiple instances, accommodating different
platforms.

(2) The technical reason is that the library instance in question is
called "BasePciExpressLib".

Given a library class called "FoobarLib", an instance that implements
the class is usually named

- with a prefix that identifies the firmware phase / module type that
the lib instance (= implementation) targets, i.e. those that the lib
instance is usable within,

- with an optional suffix that hints at the implementation details /
behavior of the library instance.

"BasePciExpressLib" has the prefix "Base", meaning that it is supposed
to be usable in all types of firmware modules, even in SEC and PEIMs --
which may not have access to writeable memory except stack (i.e.
writeable global variables). Therefore modifying the original lib
instance so that it depend on writeable globals wouldn't have been right.

We could have attempted to add the new library instance under MdePkg,
and not ArmVirtPkg, but that's just a (more difficult) special case of
point (1).

(Obviously if you try to apply the nomenclature I describe above to
"BaseCachingPciExpressLib" as well, you'll see that it doesn't match.
And that's because, when I invented the name for that lib instance, in
2015, I didn't know about the naming rules myself. :) In reality the lib
instance should be called "DxePciExpressLibCaching" -- with "Dxe" for
prefix, and "Caching" for suffix.)

Thanks
Laszlo

> If PcdPciExpressBaseAddress is fixed and doesn’t change dynamically, then the 
> caching of the value in a global var
> should be harmless (besides adding an extra read from global-var). If it does 
> change dynamically, 
> MdePkg/Library/BasePciExpressLib have the issue discussed in this email 
> thread.
> 
> Thanks,
> -Liran
___
edk2-devel mailing list

Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

[Wang, Jian J]

Hi Ard,

Sorry for the title which misleads you (I used a wrong word too. Shame!).
The real problem this patch series try to address is the confusion in these
two PCDs, PcdSetNxForStack and PcdDxeNxMemoryProtectionPolicy.
One of them will enable NX feature in cpu but another won’t. And there’s
also functionality overlap between them because stack memory is actually
type of EfiBootServiceData, which is also controlled by BIT4 of
PcdDxeNxMemoryProtectionPolicy.

Regards,
Jian

From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Tuesday, September 11, 2018 4:58 PM
To: Wang, Jian J ; Laszlo Ersek ; 
Charles Garcia-Tobin ; Leif Lindholm 

Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

On 11 September 2018 at 07:16, Jian J Wang 
mailto:jian.j.w...@intel.com>> wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since the stack memory is allocated as EfiBootServicesData, its NX protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
>

I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.


> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
>
>  ArmVirtPkg/ArmVirt.dsc.inc   |  5 -
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++---
>  MdeModulePkg/MdeModulePkg.dec| 10 +-
>  MdeModulePkg/MdeModulePkg.uni| 10 +-
>  OvmfPkg/OvmfPkgIa32.dsc  |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc   |  1 -
>  OvmfPkg/OvmfPkgX64.dsc   |  1 -
>  OvmfPkg/PlatformPei/Platform.c   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf  |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
>
> --
> 2.16.2.windows.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

[Ni, Ruiyu]

On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:

On 11 September 2018 at 07:16, Jian J Wang  wrote:

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

Since the stack memory is allocated as EfiBootServicesData, its NX protection
can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
table entries mapping the stack memory.



I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

Ard,
Are you saying you want the capability of setting certain range of BS 
data as executable? Why does ARM need such capability?


In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.



Jian J Wang (5):
   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
   OvmfPkg: expire the use of PcdSetNxForStack
   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
   MdeModulePkg: expire PcdSetNxForStack

  ArmVirtPkg/ArmVirt.dsc.inc   |  5 -
  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +-
  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  2 +-
  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++---
  MdeModulePkg/MdeModulePkg.dec| 10 +-
  MdeModulePkg/MdeModulePkg.uni| 10 +-
  OvmfPkg/OvmfPkgIa32.dsc  |  1 -
  OvmfPkg/OvmfPkgIa32X64.dsc   |  1 -
  OvmfPkg/OvmfPkgX64.dsc   |  1 -
  OvmfPkg/PlatformPei/Platform.c   |  1 -
  OvmfPkg/PlatformPei/PlatformPei.inf  |  1 -
  13 files changed, 22 insertions(+), 35 deletions(-)

--
2.16.2.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel




--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/5] MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack

[Ni, Ruiyu]

On 9/11/2018 1:16 PM, Jian J Wang wrote:

+if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT4) != 0


I suggest to use (1 << EfiBootServicesData) to replace BIT4.



--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

[Ard Biesheuvel]

On 11 September 2018 at 07:16, Jian J Wang  wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since the stack memory is allocated as EfiBootServicesData, its NX protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
>

I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.


> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
>
>  ArmVirtPkg/ArmVirt.dsc.inc   |  5 -
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++---
>  MdeModulePkg/MdeModulePkg.dec| 10 +-
>  MdeModulePkg/MdeModulePkg.uni| 10 +-
>  OvmfPkg/OvmfPkgIa32.dsc  |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc   |  1 -
>  OvmfPkg/OvmfPkgX64.dsc   |  1 -
>  OvmfPkg/PlatformPei/Platform.c   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf  |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
>
> --
> 2.16.2.windows.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch][edk2-platforms/devel-IntelAtomProcessorE3900] Leaf Hill eMMC Speed.

[zwei4]

UDK2018 eMMC driver may fail to switch eMMC to HS400 mode. This platform 
temporary solution limits eMMC controller to maximal HS200 mode.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: David Wei 
CC: Mike Wu  
CC: Mang Guo 
---
 .../Board/LeafHill/BoardInitPostMem/BoardInit.c  | 12 ++--
 .../Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h | 10 +-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
index 729b15f966..f0f89f3867 100644
--- a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
+++ b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
@@ -1,7 +1,7 @@
 /** @file
   Board Init driver.
 
-  Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
@@ -60,6 +60,7 @@ LeafHillPostMemInitCallback (
   UINT8ResetType;
   UINTNBufferSize;
   UINT8MaxPkgCState;
+  UINT8MaxSpeed;
   UINTNVariableSize;
   EFI_PEI_READ_ONLY_VARIABLE2_PPI  *VariableServices;
   SYSTEM_CONFIGURATION SystemConfiguration;
@@ -135,7 +136,14 @@ LeafHillPostMemInitCallback (
   //
   // Set PcdeMMCHostMaxSpeed
   //
-  PcdSet8 (PcdeMMCHostMaxSpeed, (UINT8) 
(SystemConfiguration.ScceMMCHostMaxSpeed));
+
+  if ((SystemConfiguration.ScceMMCHostMaxSpeed == 0) || 
(SystemConfiguration.ScceMMCHostMaxSpeed == 1)) {
+MaxSpeed = EMMC_HS200_MODE;
+PcdSet8 (PcdeMMCHostMaxSpeed, (UINT8) MaxSpeed);
+  } else {
+MaxSpeed = EMMC_DDR50_MODE;
+PcdSet8 (PcdeMMCHostMaxSpeed, (UINT8) MaxSpeed);
+  }
 
   //
   // HDA audio verb table
diff --git 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
index c1ba128709..da7ccd39ba 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
+++ 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
@@ -2,7 +2,7 @@
   Multiplatform initialization header file.
   This file includes package header files, library classes.
 
-  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
@@ -92,6 +92,14 @@
 #define MAX_PKG_CSTATE_C0 0x00
 #define MAX_PKG_CSTATE_C1 0x01
 #define MAX_PKG_CSTATE_C2 0x02
+
+//
+// eMMCHostMaxSpeed identifier.
+//
+#define EMMC_HS400_MODE   0x00
+#define EMMC_HS200_MODE   0x01
+#define EMMC_DDR50_MODE   0x02
+
 EFI_STATUS
 LeafHillGetPlatformInfoHob (
   IN CONST EFI_PEI_SERVICES **PeiServices,
-- 
2.14.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel