Date   

[PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

Pete Batard
 

The EBC Debugger [1], which was present in Tianocore [2], is an invaluable tool for EBC development.
This patch adds it back into the EDK2, allowing, for instance, the compilation of an AARCH64 EBC debugger.

Note 1: The patch is split in two, so that the changes to the existing EbcDxe code are clearer.

Note 2: The diff between the original and the new sources can be found at [3]. Most of the changes were for whitespaces/API names/compiler warnings, with the notable exception of:
- Bumping of the EBCdebugger version to 1.0.
- Dropping of the DebuggerConfiguration protocol (which would require introducing a new global GUID and protocol). I didn't see it as particularly useful to caary on and would rather see if there is actual demand for it, before adding it back.
- Add of an EFIAPI qualifier for most of the support functions, especially the ones dealing with VPrint() ouput. This is required to avoid garbage text output in some instances.
- Fixing of the erroneous display of 32 and 64 bit indexes in the disassembly.
- Replacement of one assignation with CopyMem() to avoid an intrinsic memcpy().

Note 3: I tested the debugger built for AARCH64 and X64 using gcc on Linux (Debian/Sid) as well as the one built for IA32 and X64 using VS2015 on Windows. I haven't tested an IA64 version for lack of a toolchain.

Regards,

/Pete

[1] http://www.uefi.org/node/550
[2] https://github.com/tianocore/edk/tree/master/Sample/Universal/Ebc/Dxe
[3] https://github.com/pbatard/EbcDebugger/commit/906e87ed6ceab1c361ba6f681bef48179baf549e


Re: [PATCH] ShellPkg/dmpstore: Support "-sfo"

Carsey, Jaben
 

Reviewed-by: Jaben Carsey <jaben.carsey@...>

-----Original Message-----
From: Ni, Ruiyu
Sent: Friday, November 11, 2016 2:14 AM
To: edk2-devel@...
Cc: Chen, Chen A <chen.a.chen@...>; Carsey, Jaben
<jaben.carsey@...>; Tapan Shah <tapandshah@...>
Subject: [PATCH] ShellPkg/dmpstore: Support "-sfo"
Importance: High

The patch adds the "-sfo" support to "dmpstore" command.

1. For "-l" (load variable from file), when the variable
content can be updated, the new variable data is displayed
in SFO format, otherwise, the new variable data is not
displayed. So that the SFO consumer can know which variables
are updated by parsing the SFO.

2. For "-d" (delete variable), when the variable can be deleted
successfully, only the variable name and GUID are displayed in
SFO but the attribute/data/data size are displayed as empty to
indicate the deletion happened; otherwise, the variable is not
displayed.

3. For displaying variable, when the variable specified by name
and GUID cannot be found, an error message is displayed; Otherwise,
the SFO is displayed.
E.g.: "dmpstore -guid GuidThatDoesntExist -sfo" produces output
as:
ShellCommand,"dmpstore"
VariableInfo,"","GuidThatDoesntExist","","",""

"dmpstore NameThatDoesntExist -guid GuidThatDoesntExist -sfo"
produces output as:
ShellCommand,"dmpstore"
dmpstore: No matching variables found. Guid GuidThatDoesntExist, Name
NameThatDoesntExist

The difference between the above 2 cases is that former one only
specifies the GUID, but the latter one specifies both name and GUID.
Since not specifying GUID means to use GlobalVariableGuid,
"dmpstore NameThatDoesntExist -sfo" produces the similar output as
latter one.
I personally prefer to always produce SFO output for both cases.
But the above behavior is the discussion result between HPE engineers.

Signed-off-by: Ruiyu Ni <ruiyu.ni@...>
Signed-off-by: Chen A Chen <chen.a.chen@...>
Cc: Jaben Carsey <jaben.carsey@...>
Cc: Tapan Shah <tapandshah@...>
---
.../Library/UefiShellDebug1CommandsLib/DmpStore.c | 269
+++++++++++++++------
.../UefiShellDebug1CommandsLib.uni | 5 +
2 files changed, 202 insertions(+), 72 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
index 3427c99..aa8ad09 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
@@ -2,7 +2,7 @@
Main file for DmpStore shell Debug1 function.

(C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.<BR>
- Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
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
@@ -82,12 +82,46 @@ GetAttrType (
}

/**
+ Convert binary to hex format string.
+
+ @param[in] BufferSize The size in bytes of the binary data.
+ @param[in] Buffer The binary data.
+ @param[in, out] HexString Hex format string.
+ @param[in] HexStringSize The size in bytes of the string.
+
+ @return The hex format string.
+**/
+CHAR16*
+BinaryToHexString (
+ IN VOID *Buffer,
+ IN UINTN BufferSize,
+ IN OUT CHAR16 *HexString,
+ IN UINTN HexStringSize
+ )
+{
+ UINTN Index;
+ UINTN StringIndex;
+
+ for (Index = 0, StringIndex = 0; Index < BufferSize; Index += 1) {
+ StringIndex +=
+ UnicodeSPrint (
+ &HexString[StringIndex],
+ HexStringSize - StringIndex * sizeof (CHAR16),
+ L"%02x",
+ ((UINT8 *) Buffer)[Index]
+ );
+ }
+ return HexString;
+}
+
+/**
Load the variable data from file and set to variable data base.

- @param[in] FileHandle The file to be read.
- @param[in] Name The name of the variables to be loaded.
- @param[in] Guid The guid of the variables to be loaded.
- @param[out] Found TRUE when at least one variable was loaded and
set.
+ @param[in] FileHandle The file to be read.
+ @param[in] Name The name of the variables to be loaded.
+ @param[in] Guid The guid of the variables to be loaded.
+ @param[out] Found TRUE when at least one variable was loaded
and set.
+ @param[in] StandardFormatOutput TRUE indicates Standard-Format
Output.

@retval SHELL_DEVICE_ERROR Cannot access the file.
@retval SHELL_VOLUME_CORRUPTED The file is in bad format.
@@ -99,7 +133,8 @@ LoadVariablesFromFile (
IN SHELL_FILE_HANDLE FileHandle,
IN CONST CHAR16 *Name,
IN CONST EFI_GUID *Guid,
- OUT BOOLEAN *Found
+ OUT BOOLEAN *Found,
+ IN BOOLEAN StandardFormatOutput
)
{
EFI_STATUS Status;
@@ -116,6 +151,7 @@ LoadVariablesFromFile (
CHAR16 *Attributes;
UINT8 *Buffer;
UINT32 Crc32;
+ CHAR16 *HexString;

Status = ShellGetFileSize (FileHandle, &FileSize);
if (EFI_ERROR (Status)) {
@@ -221,11 +257,6 @@ LoadVariablesFromFile (
((Guid == NULL) || CompareGuid (&Variable->Guid, Guid))
) {
Attributes = GetAttrType (Variable->Attributes);
- ShellPrintHiiEx (
- -1, -1, NULL, STRING_TOKEN(STR_DMPSTORE_HEADER_LINE),
gShellDebug1HiiHandle,
- Attributes, &Variable->Guid, Variable->Name, Variable->DataSize
- );
- SHELL_FREE_NON_NULL(Attributes);

*Found = TRUE;
Status = gRT->SetVariable (
@@ -236,8 +267,38 @@ LoadVariablesFromFile (
Variable->Data
);
if (EFI_ERROR (Status)) {
- ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_LOAD_GEN_FAIL), gShellDebug1HiiHandle, L"dmpstore",
Variable->Name, Status);
+ if (StandardFormatOutput) {
+ //
+ // Supress SFO to indicate the loading is failed.
+ //
+ } else {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_LOAD_GEN_FAIL),
gShellDebug1HiiHandle,
+ L"dmpstore", Variable->Name, Status
+ );
+ }
+ } else {
+ if (StandardFormatOutput) {
+ HexString = AllocatePool ((Variable->DataSize * 2 + 1) * sizeof
(CHAR16));
+ if (HexString != NULL) {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_VAR_SFO),
gShellDebug1HiiHandle,
+ Variable->Name, &Variable->Guid, Variable->Attributes, Variable-
DataSize,
+ BinaryToHexString (
+ Variable->Data, Variable->DataSize, HexString,
+ (Variable->DataSize * 2 + 1) * sizeof (CHAR16)
+ )
+ );
+ FreePool (HexString);
+ }
+ } else {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_HEADER_LINE),
gShellDebug1HiiHandle,
+ Attributes, &Variable->Guid, Variable->Name, Variable->DataSize
+ );
+ }
}
+ SHELL_FREE_NON_NULL(Attributes);
}
}

@@ -350,14 +411,15 @@ AppendSingleVariableToFile (

This is necessary since once a delete happens GetNextVariableName() will
work.

- @param[in] Name The variable name of the EFI variable (or NULL).
- @param[in] Guid The GUID of the variable set (or NULL).
- @param[in] Type The operation type.
- @param[in] FileHandle The file to operate on (or NULL).
- @param[in] PrevName The previous variable name from
GetNextVariableName. L"" to start.
- @param[in] FoundVarGuid The previous GUID from
GetNextVariableName. ignored at start.
- @param[in] FoundOne If a VariableName or Guid was specified and one
was printed or
- deleted, then set this to TRUE, otherwise ignored.
+ @param[in] Name The variable name of the EFI variable (or NULL).
+ @param[in] Guid The GUID of the variable set (or NULL).
+ @param[in] Type The operation type.
+ @param[in] FileHandle The file to operate on (or NULL).
+ @param[in] PrevName The previous variable name from
GetNextVariableName. L"" to start.
+ @param[in] FoundVarGuid The previous GUID from
GetNextVariableName. ignored at start.
+ @param[in] FoundOne If a VariableName or Guid was specified and
one was printed or
+ deleted, then set this to TRUE, otherwise ignored.
+ @param[in] StandardFormatOutput TRUE indicates Standard-Format
Output.

@retval SHELL_SUCCESS The operation was successful.
@retval SHELL_OUT_OF_RESOURCES A memorty allocation failed.
@@ -373,7 +435,8 @@ CascadeProcessVariables (
IN EFI_FILE_PROTOCOL *FileHandle OPTIONAL,
IN CONST CHAR16 * CONST PrevName,
IN EFI_GUID FoundVarGuid,
- IN BOOLEAN *FoundOne
+ IN BOOLEAN *FoundOne,
+ IN BOOLEAN StandardFormatOutput
)
{
EFI_STATUS Status;
@@ -383,7 +446,8 @@ CascadeProcessVariables (
UINT32 Atts;
SHELL_STATUS ShellStatus;
UINTN NameSize;
- CHAR16 *RetString;
+ CHAR16 *AttrString;
+ CHAR16 *HexString;

if (ShellGetExecutionBreakFlag()) {
return (SHELL_ABORTED);
@@ -427,7 +491,7 @@ CascadeProcessVariables (
//
// Recurse to the next iteration. We know "our" variable's name.
//
- ShellStatus = CascadeProcessVariables(Name, Guid, Type, FileHandle,
FoundVarName, FoundVarGuid, FoundOne);
+ ShellStatus = CascadeProcessVariables (Name, Guid, Type, FileHandle,
FoundVarName, FoundVarGuid, FoundOne, StandardFormatOutput);

if (ShellGetExecutionBreakFlag() || (ShellStatus == SHELL_ABORTED)) {
SHELL_FREE_NON_NULL(FoundVarName);
@@ -459,50 +523,86 @@ CascadeProcessVariables (
Status = gRT->GetVariable (FoundVarName, &FoundVarGuid, &Atts,
&DataSize, DataBuffer);
}
}
- if ((Type == DmpStoreDisplay) || (Type == DmpStoreSave)) {
//
// Last error check then print this variable out.
//
+ if (Type == DmpStoreDisplay) {
+ if (!EFI_ERROR(Status) && (DataBuffer != NULL) && (FoundVarName !=
NULL)) {
+ AttrString = GetAttrType(Atts);
+ if (StandardFormatOutput) {
+ HexString = AllocatePool ((DataSize * 2 + 1) * sizeof (CHAR16));
+ if (HexString != NULL) {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_VAR_SFO),
gShellDebug1HiiHandle,
+ FoundVarName, &FoundVarGuid, Atts, DataSize,
+ BinaryToHexString (
+ DataBuffer, DataSize, HexString, (DataSize * 2 + 1) * sizeof (CHAR16)
+ )
+ );
+ FreePool (HexString);
+ } else {
+ Status = EFI_OUT_OF_RESOURCES;
+ }
+ } else {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_HEADER_LINE),
gShellDebug1HiiHandle,
+ AttrString, &FoundVarGuid, FoundVarName, DataSize
+ );
+ DumpHex (2, 0, DataSize, DataBuffer);
+ }
+ SHELL_FREE_NON_NULL (AttrString);
+ }
+ } else if (Type == DmpStoreSave) {
if (!EFI_ERROR(Status) && (DataBuffer != NULL) && (FoundVarName !=
NULL)) {
- RetString = GetAttrType(Atts);
- ShellPrintHiiEx(
- -1,
- -1,
- NULL,
- STRING_TOKEN(STR_DMPSTORE_HEADER_LINE),
- gShellDebug1HiiHandle,
- RetString,
- &FoundVarGuid,
- FoundVarName,
- DataSize);
- if (Type == DmpStoreDisplay) {
- DumpHex(2, 0, DataSize, DataBuffer);
+ AttrString = GetAttrType (Atts);
+ if (StandardFormatOutput) {
+ HexString = AllocatePool ((DataSize * 2 + 1) * sizeof (CHAR16));
+ if (HexString != NULL) {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_VAR_SFO),
gShellDebug1HiiHandle,
+ FoundVarName, &FoundVarGuid, Atts, DataSize,
+ BinaryToHexString (
+ DataBuffer, DataSize, HexString, (DataSize * 2 + 1) * sizeof (CHAR16)
+ )
+ );
+ FreePool (HexString);
+ } else {
+ Status = EFI_OUT_OF_RESOURCES;
+ }
} else {
- Status = AppendSingleVariableToFile (
- FileHandle,
- FoundVarName,
- &FoundVarGuid,
- Atts,
- (UINT32) DataSize,
- DataBuffer
- );
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_HEADER_LINE),
gShellDebug1HiiHandle,
+ AttrString, &FoundVarGuid, FoundVarName, DataSize
+ );
}
- SHELL_FREE_NON_NULL(RetString);
+ Status = AppendSingleVariableToFile (
+ FileHandle,
+ FoundVarName,
+ &FoundVarGuid,
+ Atts,
+ (UINT32) DataSize,
+ DataBuffer
+ );
+ SHELL_FREE_NON_NULL (AttrString);
}
} else if (Type == DmpStoreDelete) {
//
// We only need name to delete it...
//
- ShellPrintHiiEx (
- -1,
- -1,
- NULL,
- STRING_TOKEN(STR_DMPSTORE_DELETE_LINE),
- gShellDebug1HiiHandle,
- &FoundVarGuid,
- FoundVarName,
- gRT->SetVariable (FoundVarName, &FoundVarGuid, Atts, 0, NULL)
- );
+ EFI_STATUS SetStatus = gRT->SetVariable (FoundVarName,
&FoundVarGuid, Atts, 0, NULL);
+ if (StandardFormatOutput) {
+ if (SetStatus == EFI_SUCCESS) {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_NG_SFO), gShellDebug1HiiHandle,
+ FoundVarName, &FoundVarGuid
+ );
+ }
+ } else {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_DELETE_LINE),
gShellDebug1HiiHandle,
+ &FoundVarGuid, FoundVarName, SetStatus
+ );
+ }
}
SHELL_FREE_NON_NULL(DataBuffer);
}
@@ -523,10 +623,11 @@ CascadeProcessVariables (
/**
Function to display or delete variables. This will set up and call into the
recursive function.

- @param[in] Name The variable name of the EFI variable (or NULL).
- @param[in] Guid The GUID of the variable set (or NULL).
- @param[in] Type The operation type.
- @param[in] FileHandle The file to save or load variables.
+ @param[in] Name The variable name of the EFI variable (or NULL).
+ @param[in] Guid The GUID of the variable set (or NULL).
+ @param[in] Type The operation type.
+ @param[in] FileHandle The file to save or load variables.
+ @param[in] StandardFormatOutput TRUE indicates Standard-Format
Output.

@retval SHELL_SUCCESS The operation was successful.
@retval SHELL_OUT_OF_RESOURCES A memorty allocation failed.
@@ -539,7 +640,8 @@ ProcessVariables (
IN CONST CHAR16 *Name OPTIONAL,
IN CONST EFI_GUID *Guid OPTIONAL,
IN DMP_STORE_TYPE Type,
- IN SHELL_FILE_HANDLE FileHandle OPTIONAL
+ IN SHELL_FILE_HANDLE FileHandle OPTIONAL,
+ IN BOOLEAN StandardFormatOutput
)
{
SHELL_STATUS ShellStatus;
@@ -550,10 +652,14 @@ ProcessVariables (
ShellStatus = SHELL_SUCCESS;
ZeroMem (&FoundVarGuid, sizeof(EFI_GUID));

+ if (StandardFormatOutput) {
+ ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN(STR_GEN_SFO_HEADER),
gShellDebug1HiiHandle, L"dmpstore");
+ }
+
if (Type == DmpStoreLoad) {
- ShellStatus = LoadVariablesFromFile (FileHandle, Name, Guid, &Found);
+ ShellStatus = LoadVariablesFromFile (FileHandle, Name, Guid, &Found,
StandardFormatOutput);
} else {
- ShellStatus = CascadeProcessVariables(Name, Guid, Type, FileHandle,
NULL, FoundVarGuid, &Found);
+ ShellStatus = CascadeProcessVariables (Name, Guid, Type, FileHandle,
NULL, FoundVarGuid, &Found, StandardFormatOutput);
}

if (!Found) {
@@ -561,13 +667,25 @@ ProcessVariables (
ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM),
gShellDebug1HiiHandle, L"dmpstore");
return (ShellStatus);
} else if (Name != NULL && Guid == NULL) {
- ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_N), gShellDebug1HiiHandle,
L"dmpstore", Name);
+ if (StandardFormatOutput) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_N_SFO), gShellDebug1HiiHandle,
Name);
+ } else {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_N), gShellDebug1HiiHandle,
L"dmpstore", Name);
+ }
} else if (Name != NULL && Guid != NULL) {
ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_GN), gShellDebug1HiiHandle,
L"dmpstore", Guid, Name);
} else if (Name == NULL && Guid == NULL) {
- ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND), gShellDebug1HiiHandle, L"dmpstore");
+ if (StandardFormatOutput) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_SFO), gShellDebug1HiiHandle);
+ } else {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND), gShellDebug1HiiHandle, L"dmpstore");
+ }
} else if (Name == NULL && Guid != NULL) {
- ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_G), gShellDebug1HiiHandle,
L"dmpstore", Guid);
+ if (StandardFormatOutput) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_G_SFO), gShellDebug1HiiHandle, Guid);
+ } else {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_DMPSTORE_NO_VAR_FOUND_G), gShellDebug1HiiHandle,
L"dmpstore", Guid);
+ }
}
return (SHELL_NOT_FOUND);
}
@@ -580,6 +698,7 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
{L"-s", TypeValue},
{L"-all", TypeFlag},
{L"-guid", TypeValue},
+ {L"-sfo", TypeFlag},
{NULL, TypeMax}
};

@@ -608,12 +727,14 @@ ShellCommandRunDmpStore (
DMP_STORE_TYPE Type;
SHELL_FILE_HANDLE FileHandle;
EFI_FILE_INFO *FileInfo;
+ BOOLEAN StandardFormatOutput;

- ShellStatus = SHELL_SUCCESS;
- Package = NULL;
- FileHandle = NULL;
- File = NULL;
- Type = DmpStoreDisplay;
+ ShellStatus = SHELL_SUCCESS;
+ Package = NULL;
+ FileHandle = NULL;
+ File = NULL;
+ Type = DmpStoreDisplay;
+ StandardFormatOutput = FALSE;

Status = ShellCommandLineParse (ParamList, &Package, &ProblemParam,
TRUE);
if (EFI_ERROR(Status)) {
@@ -742,6 +863,10 @@ ShellCommandRunDmpStore (
} else if (ShellCommandLineGetFlag(Package, L"-d")) {
Type = DmpStoreDelete;
}
+
+ if (ShellCommandLineGetFlag (Package,L"-sfo")) {
+ StandardFormatOutput = TRUE;
+ }
}

if (ShellStatus == SHELL_SUCCESS) {
@@ -750,7 +875,7 @@ ShellCommandRunDmpStore (
} else if (Type == DmpStoreLoad) {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_LOAD),
gShellDebug1HiiHandle, File);
}
- ShellStatus = ProcessVariables (Name, Guid, Type, FileHandle);
+ ShellStatus = ProcessVariables (Name, Guid, Type, FileHandle,
StandardFormatOutput);
if ((Type == DmpStoreLoad) || (Type == DmpStoreSave)) {
ShellCloseFile (&FileHandle);
}
diff --git
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
dsLib.uni
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
dsLib.uni
index 52c2af0..c97bd62 100644
---
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
dsLib.uni
+++
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
dsLib.uni
@@ -407,9 +407,14 @@
#string STR_DMPSTORE_HEADER_LINE #language en-US "Variable
%H%s%N '%H%g%N:%H%s%N' DataSize = 0x%02x\r\n"
#string STR_DMPSTORE_DELETE_LINE #language en-US "Delete variable
'%H%g%N:%H%s%N': %r\r\n"
#string STR_DMPSTORE_NO_VAR_FOUND #language en-US "%H%s%N:
No matching variables found.\r\n"
+#string STR_DMPSTORE_NO_VAR_FOUND_SFO #language en-US
"VariableInfo,\"\",\"\",\"\",\"\",\"\"\r\n"
#string STR_DMPSTORE_NO_VAR_FOUND_GN #language en-US
"%H%s%N: No matching variables found. Guid %g, Name %s\r\n"
+#string STR_DMPSTORE_NO_VAR_FOUND_NG_SFO #language en-US
"VariableInfo,\"%s\",\"%g\",\"\",\"\",\"\"\r\n"
#string STR_DMPSTORE_NO_VAR_FOUND_N #language en-US "%H%s%N:
No matching variables found. Name %s\r\n"
+#string STR_DMPSTORE_NO_VAR_FOUND_N_SFO #language en-US
#language en-US "VariableInfo,\"%s\",\"\",\"\",\"\",\"\"\r\n"
#string STR_DMPSTORE_NO_VAR_FOUND_G #language en-US "%H%s%N:
No matching variables found. Guid %g\r\n"
+#string STR_DMPSTORE_NO_VAR_FOUND_G_SFO #language en-US
"VariableInfo,\"\",\"%g\",\"\",\"\",\"\"\r\n"
+#string STR_DMPSTORE_VAR_SFO #language en-US
"VariableInfo,\"%s\",\"%g\",\"0x%x\",\"0x%x\",\"%s\"\r\n"

#string STR_GET_HELP_COMP #language en-US ""
".TH comp 0 "Compare 2 files"\r\n"
--
2.9.0.windows.1


Re: File mode problem on Github edk2-BaseTools-win32

Liming Gao
 

Evan:
Yes. I expect you send the patch into this mail list. After I review and test, I will help push it into edk2-BaseTools-win32 repo. As you mention, this repo is still the mirror of svn. Any change in this repo will impact the mirror sync. So, we expect the patch to be applied.

Thanks
Liming
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf Of Evan Lloyd
Sent: Friday, November 11, 2016 9:13 PM
To: Laszlo Ersek <lersek@...>; edk2-devel (edk2-devel@...) <edk2-devel@...>
Cc: Gao, Liming <liming.gao@...>; Leif Lindholm <leif.lindholm@...>
Subject: Re: [edk2] File mode problem on Github edk2-BaseTools-win32

Hi Laszlo.

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: 11 November 2016 11:24
To: Evan Lloyd; edk2-devel (edk2-devel@...<mailto:edk2-devel@...>)
Cc: Leif Lindholm; liming.gao@...<mailto:liming.gao@...>
Subject: Re: [edk2] File mode problem on Github edk2-BaseTools-win32
...
Liming,
Because this is purely a permission problem in the Git repository, and .exe
files are not amenable to patching,

They are -- I think if you change the file mode bits, git will see that, and will
create a patch that has no content hunks, just the file mode changes.
A semantic quibble; ".exe files are not amenable to patching" is true, the file ATTRIBUTES may be.
Patches against a .exe (or .dll) should surely start alarm bells ringing for most people.


For example, in the BaseTools/Conf/ directory, we happen have two
template files that have gratuitous execute permissions. If I remove those
permissions, "git diff" shows
...
(The above patch is one I could submit genuinely, but I'm too lazy. :))
And somebody on the list would only object, so why bother? ;-)


I have raised a pull request on https://github.com/tianocore/edk2-
BaseTools-win32/pulls
This is only a minor thing, but I would deem it a great favour were you to
accept the pull request.
It has me tearing my hair out, and I have little enough to begin with. :-{
It is fine to send pull requests, but:
- they should be mailed to the list (not opened on github),
- the patches have to be reviewed first, anyway.

(Speaking about the edk2 repo at least -- I realize this is a different repo.)
As you point out, this is for a different repo; provided (I think) as a convenience, and is ancillary to edk2.
My viewpoint is that this is a specialised aspect, of interest to very few people. (Does anyone else use Cygwin Git and the Win32 binaries?)
The only reason for publishing this request here was one of awareness. Most people will, I expect, be blissfully unconcerned.

I am happy to submit a patch though, should those responsible (Liming?) want that.
Until that is confirmed though, I'm assuming that the GitHub repo is a mirror of a Subversion original (which will not record modes), so applying a patch might involve a lot more work than accepting the pull request on GitHub.

Regards,
Evan


Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@...<mailto:edk2-devel@...>
https://lists.01.org/mailman/listinfo/edk2-devel


Re: File mode problem on Github edk2-BaseTools-win32

Evan Lloyd <Evan.Lloyd@...>
 

Hi Laszlo.

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: 11 November 2016 11:24
To: Evan Lloyd; edk2-devel (edk2-devel@...)
Cc: Leif Lindholm; liming.gao@...
Subject: Re: [edk2] File mode problem on Github edk2-BaseTools-win32
...
Liming,
Because this is purely a permission problem in the Git repository, and .exe
files are not amenable to patching,

They are -- I think if you change the file mode bits, git will see that, and will
create a patch that has no content hunks, just the file mode changes.
A semantic quibble; ".exe files are not amenable to patching" is true, the file ATTRIBUTES may be.
Patches against a .exe (or .dll) should surely start alarm bells ringing for most people.


For example, in the BaseTools/Conf/ directory, we happen have two
template files that have gratuitous execute permissions. If I remove those
permissions, "git diff" shows
...
(The above patch is one I could submit genuinely, but I'm too lazy. :))
And somebody on the list would only object, so why bother? ;-)


I have raised a pull request on https://github.com/tianocore/edk2-
BaseTools-win32/pulls
This is only a minor thing, but I would deem it a great favour were you to
accept the pull request.
It has me tearing my hair out, and I have little enough to begin with. :-{
It is fine to send pull requests, but:
- they should be mailed to the list (not opened on github),
- the patches have to be reviewed first, anyway.

(Speaking about the edk2 repo at least -- I realize this is a different repo.)
As you point out, this is for a different repo; provided (I think) as a convenience, and is ancillary to edk2.
My viewpoint is that this is a specialised aspect, of interest to very few people. (Does anyone else use Cygwin Git and the Win32 binaries?)
The only reason for publishing this request here was one of awareness. Most people will, I expect, be blissfully unconcerned.

I am happy to submit a patch though, should those responsible (Liming?) want that.
Until that is confirmed though, I'm assuming that the GitHub repo is a mirror of a Subversion original (which will not record modes), so applying a patch might involve a lot more work than accepting the pull request on GitHub.

Regards,
Evan


Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [PATCH V3 0/6] Enable SMM page level protection.

Laszlo Ersek
 

On 11/11/16 13:53, Yao, Jiewen wrote:
Sorry, I did not explain it clear enough before.

Jeff is right. The NX fix is introduced in this patch series.
The reason is that when we update page table to protect the SMRAM, we would like enable the protection as early as possible.
We moved the NX enabling code from C function to ASM function to achieve that.

You can see my GIT message in 5/6.
==============
The XD enabling code is moved to SmiEntry to let NX take effect.
==============

Unfortunately, I introduced a bug there. You help to catch it and now I fix it.
It is not related to current code.

What about your idea?
Right, I missed that the XD handling in SmiEntry.nasm was brand new code
added by this series. So, the current structure of both series should be
fine; I should be able to start testing them (hopefully) soon.

Thanks!
Laszlo

Thank you
Yao Jiewen

From: Fan, Jeff
Sent: Friday, November 11, 2016 8:38 PM
To: Laszlo Ersek <lersek@...>; Yao, Jiewen <jiewen.yao@...>; edk2-devel@... <edk2-devel@...>
Cc: Tian, Feng <feng.tian@...>; Kinney, Michael D <michael.d.kinney@...>; Paolo Bonzini <pbonzini@...>; Zeng, Star <star.zeng@...>
Subject: RE: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Laszlo,

NX support and fix in SmiEntry.asm is new code for UefiCpuPkg. So, that means we still have other unknown problem on S3 boot issue.

Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:26 PM
To: Yao, Jiewen; edk2-devel@...<mailto:edk2-devel@...>; Fan, Jeff
Cc: Tian, Feng; Kinney, Michael D; Paolo Bonzini; Zeng, Star
Subject: Re: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Jiewen, Jeff,

On 11/11/16 10:12, Yao, Jiewen wrote:
HI Laszlo
I fixed the IA32 boot issue in this patch
Thank you.

Jiewen, I'd like to request the following:

- Please separate the fix for the incorrect parameter passing to
SmiRendezvous() out to a separate patch. It is my understanding that
the issue exists already in the master branch. Is that right? If it
is, then the fix should not be tied to the SMM page level protection
feature.

- Please give that patch to Jeff.

Jeff,

can you please repost your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

to edk2-devel, as v3, with Jiewen's patch from above included, as patch#4? Because, I would like to see a patch series that addresses all known S3 issues that we've uncovered in this investigation.

The first three patches should fix BZ#216, yes.

The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be hit during S3.

My goal is to apply that series (the first 3 patches from Jeff, and the fourth patch from Jiewen), and to test it as one unit. I'd like to see if those changes fix the infrequent, but still triggerable issues with
S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
series should be committed.

After that, I'd like to test Jiewen's v4 series for the SMM page level protection, separately.

In other words, first we should fix the existent bugs that Jiewen's SMM page level protection feature only amplifies (but doesn't introduce) on QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the new feature.

Would this work for you guys?

Thank you,
Laszlo

with DEBUG message update you suggested.

My unit test failed before. Now it can pass.
I validated on a real IA32 and Windows OVMF with and without XD.


For QEMU installation, it is still on progress.
We have setup a Fedora 24 host, download QEMU, and install it.
But we are still struggling to make QEMU boot on Fedora.
Your step by step is great. There is still some minor place we stuck in due to my ignorance.
My goal is still to setup an environment like yours for our validation or issue reproduce.
It just need take some time, more than I expected. sign...

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf
Of Jiewen Yao
Sent: Friday, November 11, 2016 5:01 PM
To: edk2-devel@...<mailto:edk2-devel@...>
Cc: Tian, Feng <feng.tian@...<mailto:feng.tian@...>>; Kinney, Michael D
<michael.d.kinney@...<mailto:michael.d.kinney@...>>; Paolo Bonzini <pbonzini@...<mailto:pbonzini@...>>;
Laszlo Ersek <lersek@...<mailto:lersek@...>>; Fan, Jeff <jeff.fan@...<mailto:jeff.fan@...>>;
Zeng, Star <star.zeng@...<mailto:star.zeng@...>>
Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.


==== below is V3 description ====
1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
(Many thanks to Laszlo Ersek <lersek@...<mailto:lersek@...>> for catching it.)
2) PiSmmCpu: Add ASSERT for CpuIndex check.
3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
4) PiSmmCpu: Do not report DEBUG message for Ap non present when
PcdCpuSmmSyncMode==1 (Relex mode).
5) PiSmmCpu: Do not report DEBUG message for AP removed when
PcdCpuHotPlugSupport==TRUE.

Tested combination:
1) XD disabled
2) XD enabled in SMM and disabled in non-SMM.
3) XD enabled in SMM and enabled in non-SMM.

==== below is V2 description ====
1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
2) PiSmmCpu: Add debug info on StartupAp() fails.
3) PiSmmCpu: Add ASSERT for AllocatePages().
4) PiSmmCpu: Add protection detail in commit message.
5) UefiCpuPkg.dsc: Add page table footprint info in commit message.

==== below is V1 description ====
This series patch enables SMM page level protection.
Features are:
1) PiSmmCore reports SMM PE image code/data information in
EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable and set XD for
data page and RO for code page.
3) PiSmmCpu enables Static Paging for X64 according to
PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G is
used as long as it is supported.
4) PiSmmCpu sets importance data structure to be read only, such as
Gdt, Idt, SmmEntrypoint, and PageTable itself.

tested platform:
1) Intel internal platform (X64).
2) EDKII Quark IA32
3) EDKII Vlv2 X64
4) EDKII OVMF IA32 and IA32X64. (with -smp 8)

Cc: Jeff Fan <jeff.fan@...<mailto:jeff.fan@...>>
Cc: Feng Tian <feng.tian@...<mailto:feng.tian@...>>
Cc: Star Zeng <star.zeng@...<mailto:star.zeng@...>>
Cc: Michael D Kinney <michael.d.kinney@...<mailto:michael.d.kinney@...>>
Cc: Laszlo Ersek <lersek@...<mailto:lersek@...>>
Cc: Paolo Bonzini <pbonzini@...<mailto:pbonzini@...>>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@...<mailto:jiewen.yao@...>>

Jiewen Yao (6):
MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
QuarkPlatformPkg/dsc: enable Smm paging protection.

MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 66 +
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 1509
++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/Page.c | 775
+++++++++-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 40
+
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 91
++
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 2
+
MdeModulePkg/Core/PiSmmCore/Pool.c | 16
+
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h | 51 +
MdeModulePkg/MdeModulePkg.dec |
3 +
QuarkPlatformPkg/Quark.dsc | 6 +
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 71
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 79
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 226
+--
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 37
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 4
+-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 135
+-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |
144 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |
156 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |
5 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
871 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 39
+-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 274
+++-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 59
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 62
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S | 250
+---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm | 35
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm | 31
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 30
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 7
+-
UefiCpuPkg/UefiCpuPkg.dec | 8 +
36 files changed, 4585 insertions(+), 803 deletions(-) create mode
100644 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
create mode 100644
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
create mode 100644
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

--
2.7.4.windows.1

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


Re: [PATCH V3 0/6] Enable SMM page level protection.

Yao, Jiewen
 

Sorry, I did not explain it clear enough before.

Jeff is right. The NX fix is introduced in this patch series.
The reason is that when we update page table to protect the SMRAM, we would like enable the protection as early as possible.
We moved the NX enabling code from C function to ASM function to achieve that.

You can see my GIT message in 5/6.
==============
The XD enabling code is moved to SmiEntry to let NX take effect.
==============

Unfortunately, I introduced a bug there. You help to catch it and now I fix it.
It is not related to current code.

What about your idea?

Thank you
Yao Jiewen

From: Fan, Jeff
Sent: Friday, November 11, 2016 8:38 PM
To: Laszlo Ersek <lersek@...>; Yao, Jiewen <jiewen.yao@...>; edk2-devel@... <edk2-devel@...>
Cc: Tian, Feng <feng.tian@...>; Kinney, Michael D <michael.d.kinney@...>; Paolo Bonzini <pbonzini@...>; Zeng, Star <star.zeng@...>
Subject: RE: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Laszlo,

NX support and fix in SmiEntry.asm is new code for UefiCpuPkg. So, that means we still have other unknown problem on S3 boot issue.

Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:26 PM
To: Yao, Jiewen; edk2-devel@...<mailto:edk2-devel@...>; Fan, Jeff
Cc: Tian, Feng; Kinney, Michael D; Paolo Bonzini; Zeng, Star
Subject: Re: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Jiewen, Jeff,

On 11/11/16 10:12, Yao, Jiewen wrote:
HI Laszlo
I fixed the IA32 boot issue in this patch
Thank you.

Jiewen, I'd like to request the following:

- Please separate the fix for the incorrect parameter passing to
SmiRendezvous() out to a separate patch. It is my understanding that
the issue exists already in the master branch. Is that right? If it
is, then the fix should not be tied to the SMM page level protection
feature.

- Please give that patch to Jeff.

Jeff,

can you please repost your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

to edk2-devel, as v3, with Jiewen's patch from above included, as patch#4? Because, I would like to see a patch series that addresses all known S3 issues that we've uncovered in this investigation.

The first three patches should fix BZ#216, yes.

The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be hit during S3.

My goal is to apply that series (the first 3 patches from Jeff, and the fourth patch from Jiewen), and to test it as one unit. I'd like to see if those changes fix the infrequent, but still triggerable issues with
S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
series should be committed.

After that, I'd like to test Jiewen's v4 series for the SMM page level protection, separately.

In other words, first we should fix the existent bugs that Jiewen's SMM page level protection feature only amplifies (but doesn't introduce) on QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the new feature.

Would this work for you guys?

Thank you,
Laszlo

with DEBUG message update you suggested.

My unit test failed before. Now it can pass.
I validated on a real IA32 and Windows OVMF with and without XD.


For QEMU installation, it is still on progress.
We have setup a Fedora 24 host, download QEMU, and install it.
But we are still struggling to make QEMU boot on Fedora.
Your step by step is great. There is still some minor place we stuck in due to my ignorance.
My goal is still to setup an environment like yours for our validation or issue reproduce.
It just need take some time, more than I expected. sign...

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf
Of Jiewen Yao
Sent: Friday, November 11, 2016 5:01 PM
To: edk2-devel@...<mailto:edk2-devel@...>
Cc: Tian, Feng <feng.tian@...<mailto:feng.tian@...>>; Kinney, Michael D
<michael.d.kinney@...<mailto:michael.d.kinney@...>>; Paolo Bonzini <pbonzini@...<mailto:pbonzini@...>>;
Laszlo Ersek <lersek@...<mailto:lersek@...>>; Fan, Jeff <jeff.fan@...<mailto:jeff.fan@...>>;
Zeng, Star <star.zeng@...<mailto:star.zeng@...>>
Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.


==== below is V3 description ====
1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
(Many thanks to Laszlo Ersek <lersek@...<mailto:lersek@...>> for catching it.)
2) PiSmmCpu: Add ASSERT for CpuIndex check.
3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
4) PiSmmCpu: Do not report DEBUG message for Ap non present when
PcdCpuSmmSyncMode==1 (Relex mode).
5) PiSmmCpu: Do not report DEBUG message for AP removed when
PcdCpuHotPlugSupport==TRUE.

Tested combination:
1) XD disabled
2) XD enabled in SMM and disabled in non-SMM.
3) XD enabled in SMM and enabled in non-SMM.

==== below is V2 description ====
1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
2) PiSmmCpu: Add debug info on StartupAp() fails.
3) PiSmmCpu: Add ASSERT for AllocatePages().
4) PiSmmCpu: Add protection detail in commit message.
5) UefiCpuPkg.dsc: Add page table footprint info in commit message.

==== below is V1 description ====
This series patch enables SMM page level protection.
Features are:
1) PiSmmCore reports SMM PE image code/data information in
EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable and set XD for
data page and RO for code page.
3) PiSmmCpu enables Static Paging for X64 according to
PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G is
used as long as it is supported.
4) PiSmmCpu sets importance data structure to be read only, such as
Gdt, Idt, SmmEntrypoint, and PageTable itself.

tested platform:
1) Intel internal platform (X64).
2) EDKII Quark IA32
3) EDKII Vlv2 X64
4) EDKII OVMF IA32 and IA32X64. (with -smp 8)

Cc: Jeff Fan <jeff.fan@...<mailto:jeff.fan@...>>
Cc: Feng Tian <feng.tian@...<mailto:feng.tian@...>>
Cc: Star Zeng <star.zeng@...<mailto:star.zeng@...>>
Cc: Michael D Kinney <michael.d.kinney@...<mailto:michael.d.kinney@...>>
Cc: Laszlo Ersek <lersek@...<mailto:lersek@...>>
Cc: Paolo Bonzini <pbonzini@...<mailto:pbonzini@...>>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@...<mailto:jiewen.yao@...>>

Jiewen Yao (6):
MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
QuarkPlatformPkg/dsc: enable Smm paging protection.

MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 66 +
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 1509
++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/Page.c | 775
+++++++++-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 40
+
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 91
++
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 2
+
MdeModulePkg/Core/PiSmmCore/Pool.c | 16
+
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h | 51 +
MdeModulePkg/MdeModulePkg.dec |
3 +
QuarkPlatformPkg/Quark.dsc | 6 +
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 71
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 79
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 226
+--
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 37
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 4
+-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 135
+-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |
144 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |
156 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |
5 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
871 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 39
+-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 274
+++-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 59
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 62
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S | 250
+---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm | 35
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm | 31
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 30
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 7
+-
UefiCpuPkg/UefiCpuPkg.dec | 8 +
36 files changed, 4585 insertions(+), 803 deletions(-) create mode
100644 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
create mode 100644
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
create mode 100644
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

--
2.7.4.windows.1

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


Re: [PATCH V3 0/6] Enable SMM page level protection.

Laszlo Ersek
 

On 11/11/16 13:38, Fan, Jeff wrote:
Laszlo,

NX support and fix in SmiEntry.asm is new code for UefiCpuPkg.
Ah, you are right. Sorry, I missed that. I've checked the v2 series now
and I see the SkipXd label and friends are introduced by v2; it's not
preexistent code.

This means it should be okay for me to test these series as they are. No
need to move patches between them.

So, that means we still have other unknown problem on S3 boot issue.
Hm, with your above explanation, I'm actually not sure about that. I
think that your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

has a good chance to fix it all. I hope I can test it today. (And once
I'm done with it, I'll move to Jiewen's v3.)

Thanks!
Laszlo



-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:26 PM
To: Yao, Jiewen; edk2-devel@...; Fan, Jeff
Cc: Tian, Feng; Kinney, Michael D; Paolo Bonzini; Zeng, Star
Subject: Re: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Jiewen, Jeff,

On 11/11/16 10:12, Yao, Jiewen wrote:
HI Laszlo
I fixed the IA32 boot issue in this patch
Thank you.

Jiewen, I'd like to request the following:

- Please separate the fix for the incorrect parameter passing to
SmiRendezvous() out to a separate patch. It is my understanding that
the issue exists already in the master branch. Is that right? If it
is, then the fix should not be tied to the SMM page level protection
feature.

- Please give that patch to Jeff.

Jeff,

can you please repost your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

to edk2-devel, as v3, with Jiewen's patch from above included, as patch#4? Because, I would like to see a patch series that addresses all known S3 issues that we've uncovered in this investigation.

The first three patches should fix BZ#216, yes.

The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be hit during S3.

My goal is to apply that series (the first 3 patches from Jeff, and the fourth patch from Jiewen), and to test it as one unit. I'd like to see if those changes fix the infrequent, but still triggerable issues with
S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
series should be committed.

After that, I'd like to test Jiewen's v4 series for the SMM page level protection, separately.

In other words, first we should fix the existent bugs that Jiewen's SMM page level protection feature only amplifies (but doesn't introduce) on QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the new feature.

Would this work for you guys?

Thank you,
Laszlo

with DEBUG message update you suggested.

My unit test failed before. Now it can pass.
I validated on a real IA32 and Windows OVMF with and without XD.


For QEMU installation, it is still on progress.
We have setup a Fedora 24 host, download QEMU, and install it.
But we are still struggling to make QEMU boot on Fedora.
Your step by step is great. There is still some minor place we stuck in due to my ignorance.
My goal is still to setup an environment like yours for our validation or issue reproduce.
It just need take some time, more than I expected. sign...

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf
Of Jiewen Yao
Sent: Friday, November 11, 2016 5:01 PM
To: edk2-devel@...
Cc: Tian, Feng <feng.tian@...>; Kinney, Michael D
<michael.d.kinney@...>; Paolo Bonzini <pbonzini@...>;
Laszlo Ersek <lersek@...>; Fan, Jeff <jeff.fan@...>;
Zeng, Star <star.zeng@...>
Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.


==== below is V3 description ====
1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
(Many thanks to Laszlo Ersek <lersek@...> for catching it.)
2) PiSmmCpu: Add ASSERT for CpuIndex check.
3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
4) PiSmmCpu: Do not report DEBUG message for Ap non present when
PcdCpuSmmSyncMode==1 (Relex mode).
5) PiSmmCpu: Do not report DEBUG message for AP removed when
PcdCpuHotPlugSupport==TRUE.

Tested combination:
1) XD disabled
2) XD enabled in SMM and disabled in non-SMM.
3) XD enabled in SMM and enabled in non-SMM.

==== below is V2 description ====
1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
2) PiSmmCpu: Add debug info on StartupAp() fails.
3) PiSmmCpu: Add ASSERT for AllocatePages().
4) PiSmmCpu: Add protection detail in commit message.
5) UefiCpuPkg.dsc: Add page table footprint info in commit message.

==== below is V1 description ====
This series patch enables SMM page level protection.
Features are:
1) PiSmmCore reports SMM PE image code/data information in
EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable and set XD for
data page and RO for code page.
3) PiSmmCpu enables Static Paging for X64 according to
PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G is
used as long as it is supported.
4) PiSmmCpu sets importance data structure to be read only, such as
Gdt, Idt, SmmEntrypoint, and PageTable itself.

tested platform:
1) Intel internal platform (X64).
2) EDKII Quark IA32
3) EDKII Vlv2 X64
4) EDKII OVMF IA32 and IA32X64. (with -smp 8)

Cc: Jeff Fan <jeff.fan@...>
Cc: Feng Tian <feng.tian@...>
Cc: Star Zeng <star.zeng@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@...>

Jiewen Yao (6):
MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
QuarkPlatformPkg/dsc: enable Smm paging protection.

MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 66 +
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 1509
++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/Page.c | 775
+++++++++-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 40
+
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 91
++
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 2
+
MdeModulePkg/Core/PiSmmCore/Pool.c | 16
+
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h | 51 +
MdeModulePkg/MdeModulePkg.dec |
3 +
QuarkPlatformPkg/Quark.dsc | 6 +
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 71
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 79
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 226
+--
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 37
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 4
+-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 135
+-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |
144 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |
156 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |
5 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
871 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 39
+-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 274
+++-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 59
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 62
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S | 250
+---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm | 35
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm | 31
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 30
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 7
+-
UefiCpuPkg/UefiCpuPkg.dec | 8 +
36 files changed, 4585 insertions(+), 803 deletions(-) create mode
100644 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
create mode 100644
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
create mode 100644
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

--
2.7.4.windows.1

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


Re: [PATCH v2 2/2] MdeModulePkg/Ip4Dxe: Correct the return status

Laszlo Ersek
 

On 11/11/16 06:18, Jiaxin Wu wrote:
This patch made the following change:
* DataItem->Status should be updated to the status code.
* Data should not be freed if EFI_NOT_READY returned.

Cc: Santhapur Naveen <naveens@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ye Ting <ting.ye@...>
Cc: Fu Siyuan <siyuan.fu@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <jiaxin.wu@...>
---
MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
index 5b01b35..88ead9d 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
@@ -1280,25 +1280,21 @@ Ip4Config2SetMaunualAddress (
DataItem->DataSize = DataSize;
DataItem->Status = EFI_NOT_READY;

IpSb->Reconfig = TRUE;
Status = Ip4Config2SetDefaultAddr (IpSb, StationAddress, SubnetMask);
- if (EFI_ERROR (Status)) {
- goto ON_EXIT;
- }

- DataItem->Status = EFI_SUCCESS;
+ DataItem->Status = Status;

-ON_EXIT:
- if (EFI_ERROR (DataItem->Status)) {
+ if (EFI_ERROR (DataItem->Status) && DataItem->Status != EFI_NOT_READY) {
if (Ptr != NULL) {
FreePool (Ptr);
}
DataItem->Data.Ptr = NULL;
}

- return EFI_SUCCESS;
+ return Status;
}

/**
The work function is to set the gateway addresses manually for the EFI IPv4
network stack that is running on the communication device that this EFI IPv4
Reviewed-by: Laszlo Ersek <lersek@...>


Re: [PATCH v2 1/2] MdeModulePkg/Ip4Dxe: Add wrong/invalid subnet check

Laszlo Ersek
 

On 11/11/16 06:18, Jiaxin Wu wrote:
v2:
* Separate out the return status fix.
* Replace IP4_MASK_MAX with IP4_MASK_MAX.
* Remove the ON_EXIT label.

This patch is used to add the wrong/invalid subnet check.

Cc: Santhapur Naveen <naveens@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ye Ting <ting.ye@...>
Cc: Fu Siyuan <siyuan.fu@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <jiaxin.wu@...>
---
MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c | 10 +++++++---
MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c | 8 +++++---
2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
index a931bb3..5b01b35 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
@@ -1253,10 +1253,17 @@ Ip4Config2SetMaunualAddress (
return EFI_WRITE_PROTECTED;
}

NewAddress = *((EFI_IP4_CONFIG2_MANUAL_ADDRESS *) Data);

+ StationAddress = EFI_NTOHL (NewAddress.Address);
+ SubnetMask = EFI_NTOHL (NewAddress.SubnetMask);
+
+ if (NetGetMaskLength (SubnetMask) == IP4_MASK_NUM) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Store the new data, and init the DataItem status to EFI_NOT_READY because
// we may have an asynchronous configuration process.
//
Ptr = AllocateCopyPool (DataSize, Data);
@@ -1271,13 +1278,10 @@ Ip4Config2SetMaunualAddress (

DataItem->Data.Ptr = Ptr;
DataItem->DataSize = DataSize;
DataItem->Status = EFI_NOT_READY;

- StationAddress = EFI_NTOHL (NewAddress.Address);
- SubnetMask = EFI_NTOHL (NewAddress.SubnetMask);
-
IpSb->Reconfig = TRUE;
Status = Ip4Config2SetDefaultAddr (IpSb, StationAddress, SubnetMask);
if (EFI_ERROR (Status)) {
goto ON_EXIT;
}
diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
index 9cd5dd5..b0cc6a3 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
@@ -562,10 +562,15 @@ Ip4SetAddress (
EFI_STATUS Status;
INTN Len;

NET_CHECK_SIGNATURE (Interface, IP4_INTERFACE_SIGNATURE);

+ Len = NetGetMaskLength (SubnetMask);
+ if (Len == IP4_MASK_NUM) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Set the ip/netmask, then compute the subnet broadcast
// and network broadcast for easy access. When computing
// nework broadcast, the subnet mask is most like longer
// than the default netmask (not subneted) as defined in
@@ -573,13 +578,10 @@ Ip4SetAddress (
// networks, use the subnet's mask instead.
//
Interface->Ip = IpAddr;
Interface->SubnetMask = SubnetMask;
Interface->SubnetBrdcast = (IpAddr | ~SubnetMask);
-
- Len = NetGetMaskLength (SubnetMask);
- ASSERT (Len <= IP4_MASK_MAX);
Interface->NetBrdcast = (IpAddr | ~SubnetMask);

//
// Do clean up for Arp child
//
Reviewed-by: Laszlo Ersek <lersek@...>


Re: [PATCH 0/2] Place APs to suitable state on Legacy OS boot

Fan, Jeff <jeff.fan@...>
 

Laszlo,

Got you. It's a good suggestion!

Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:33 PM
To: Fan, Jeff
Cc: edk2-devel@...; Tian, Feng; Yao, Jiewen; Kinney, Michael D; Paolo Bonzini
Subject: Re: [edk2] [PATCH 0/2] Place APs to suitable state on Legacy OS boot

On 11/11/16 12:57, Jeff Fan wrote:
Currently, DxeMpLib only places APs into specified c-state in Exit
Boot Service callback function for UEFI OS boot. We need to put APs
into specified c-state for legacy OS boot also.

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

Jeff Fan (2):
UefiCpuPkg/DxeMpLib: Rename MpInitExitBootServicesCallback()
UefiCpuPkg/DxeMpLib: Place APs to suitable state on Legacy OS boot

UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 18 +++++++++++++++---
2 files changed, 16 insertions(+), 3 deletions(-)
From a cursory look, it looks good to me, but I think I'll leave the official review on this to Mike et al :)

Also, a meta-hint: when you post a cover letter for a patch series, it is best to collect all the CC's from the patches, sort them uniquely, and then add the resulting CC list to the cover letter as well. This way every CC'd person will receive the high level description for the entire set.

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

* finally, add all of the Cc: tags to the cover letter that you
used across all of the patches. This will ensure that even if a
maintainer is involved in reviewing one or two of your patches
across the series, he or she will get a copy of your cover
letter, which outlines the full feature or bugfix.

Thanks,
Laszlo


Re: [PATCH V3 0/6] Enable SMM page level protection.

Fan, Jeff <jeff.fan@...>
 

Laszlo,

NX support and fix in SmiEntry.asm is new code for UefiCpuPkg. So, that means we still have other unknown problem on S3 boot issue.

Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:26 PM
To: Yao, Jiewen; edk2-devel@...; Fan, Jeff
Cc: Tian, Feng; Kinney, Michael D; Paolo Bonzini; Zeng, Star
Subject: Re: [edk2] [PATCH V3 0/6] Enable SMM page level protection.

Jiewen, Jeff,

On 11/11/16 10:12, Yao, Jiewen wrote:
HI Laszlo
I fixed the IA32 boot issue in this patch
Thank you.

Jiewen, I'd like to request the following:

- Please separate the fix for the incorrect parameter passing to
SmiRendezvous() out to a separate patch. It is my understanding that
the issue exists already in the master branch. Is that right? If it
is, then the fix should not be tied to the SMM page level protection
feature.

- Please give that patch to Jeff.

Jeff,

can you please repost your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

to edk2-devel, as v3, with Jiewen's patch from above included, as patch#4? Because, I would like to see a patch series that addresses all known S3 issues that we've uncovered in this investigation.

The first three patches should fix BZ#216, yes.

The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be hit during S3.

My goal is to apply that series (the first 3 patches from Jeff, and the fourth patch from Jiewen), and to test it as one unit. I'd like to see if those changes fix the infrequent, but still triggerable issues with
S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
series should be committed.

After that, I'd like to test Jiewen's v4 series for the SMM page level protection, separately.

In other words, first we should fix the existent bugs that Jiewen's SMM page level protection feature only amplifies (but doesn't introduce) on QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the new feature.

Would this work for you guys?

Thank you,
Laszlo

with DEBUG message update you suggested.

My unit test failed before. Now it can pass.
I validated on a real IA32 and Windows OVMF with and without XD.


For QEMU installation, it is still on progress.
We have setup a Fedora 24 host, download QEMU, and install it.
But we are still struggling to make QEMU boot on Fedora.
Your step by step is great. There is still some minor place we stuck in due to my ignorance.
My goal is still to setup an environment like yours for our validation or issue reproduce.
It just need take some time, more than I expected. sign...

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf
Of Jiewen Yao
Sent: Friday, November 11, 2016 5:01 PM
To: edk2-devel@...
Cc: Tian, Feng <feng.tian@...>; Kinney, Michael D
<michael.d.kinney@...>; Paolo Bonzini <pbonzini@...>;
Laszlo Ersek <lersek@...>; Fan, Jeff <jeff.fan@...>;
Zeng, Star <star.zeng@...>
Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.


==== below is V3 description ====
1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
(Many thanks to Laszlo Ersek <lersek@...> for catching it.)
2) PiSmmCpu: Add ASSERT for CpuIndex check.
3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
4) PiSmmCpu: Do not report DEBUG message for Ap non present when
PcdCpuSmmSyncMode==1 (Relex mode).
5) PiSmmCpu: Do not report DEBUG message for AP removed when
PcdCpuHotPlugSupport==TRUE.

Tested combination:
1) XD disabled
2) XD enabled in SMM and disabled in non-SMM.
3) XD enabled in SMM and enabled in non-SMM.

==== below is V2 description ====
1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
2) PiSmmCpu: Add debug info on StartupAp() fails.
3) PiSmmCpu: Add ASSERT for AllocatePages().
4) PiSmmCpu: Add protection detail in commit message.
5) UefiCpuPkg.dsc: Add page table footprint info in commit message.

==== below is V1 description ====
This series patch enables SMM page level protection.
Features are:
1) PiSmmCore reports SMM PE image code/data information in
EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable and set XD for
data page and RO for code page.
3) PiSmmCpu enables Static Paging for X64 according to
PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G is
used as long as it is supported.
4) PiSmmCpu sets importance data structure to be read only, such as
Gdt, Idt, SmmEntrypoint, and PageTable itself.

tested platform:
1) Intel internal platform (X64).
2) EDKII Quark IA32
3) EDKII Vlv2 X64
4) EDKII OVMF IA32 and IA32X64. (with -smp 8)

Cc: Jeff Fan <jeff.fan@...>
Cc: Feng Tian <feng.tian@...>
Cc: Star Zeng <star.zeng@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@...>

Jiewen Yao (6):
MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
QuarkPlatformPkg/dsc: enable Smm paging protection.

MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 66 +
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 1509
++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/Page.c | 775
+++++++++-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 40
+
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 91
++
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 2
+
MdeModulePkg/Core/PiSmmCore/Pool.c | 16
+
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h | 51 +
MdeModulePkg/MdeModulePkg.dec |
3 +
QuarkPlatformPkg/Quark.dsc | 6 +
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 71
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 79
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 226
+--
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 37
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 4
+-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 135
+-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |
144 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |
156 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |
5 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
871 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 39
+-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 274
+++-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 59
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 62
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S | 250
+---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm | 35
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm | 31
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 30
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 7
+-
UefiCpuPkg/UefiCpuPkg.dec | 8 +
36 files changed, 4585 insertions(+), 803 deletions(-) create mode
100644 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
create mode 100644
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
create mode 100644
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

--
2.7.4.windows.1

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


Re: [PATCH 0/2] Place APs to suitable state on Legacy OS boot

Laszlo Ersek
 

On 11/11/16 12:57, Jeff Fan wrote:
Currently, DxeMpLib only places APs into specified c-state in Exit Boot Service
callback function for UEFI OS boot. We need to put APs into specified c-state
for legacy OS boot also.

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

Jeff Fan (2):
UefiCpuPkg/DxeMpLib: Rename MpInitExitBootServicesCallback()
UefiCpuPkg/DxeMpLib: Place APs to suitable state on Legacy OS boot

UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 18 +++++++++++++++---
2 files changed, 16 insertions(+), 3 deletions(-)
From a cursory look, it looks good to me, but I think I'll leave the
official review on this to Mike et al :)

Also, a meta-hint: when you post a cover letter for a patch series, it
is best to collect all the CC's from the patches, sort them uniquely,
and then add the resulting CC list to the cover letter as well. This way
every CC'd person will receive the high level description for the entire
set.

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

* finally, add all of the Cc: tags to the cover letter that you
used across all of the patches. This will ensure that even if a
maintainer is involved in reviewing one or two of your patches
across the series, he or she will get a copy of your cover
letter, which outlines the full feature or bugfix.

Thanks,
Laszlo


Re: [PATCH V3 0/6] Enable SMM page level protection.

Laszlo Ersek
 

Jiewen, Jeff,

On 11/11/16 10:12, Yao, Jiewen wrote:
HI Laszlo
I fixed the IA32 boot issue in this patch
Thank you.

Jiewen, I'd like to request the following:

- Please separate the fix for the incorrect parameter passing to
SmiRendezvous() out to a separate patch. It is my understanding that
the issue exists already in the master branch. Is that right? If it
is, then the fix should not be tied to the SMM page level protection
feature.

- Please give that patch to Jeff.

Jeff,

can you please repost your series

[edk2] [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

to edk2-devel, as v3, with Jiewen's patch from above included, as
patch#4? Because, I would like to see a patch series that addresses all
known S3 issues that we've uncovered in this investigation.

The first three patches should fix BZ#216, yes.

The last (4th) patch, from Jiewen, is unrelated to that BZ indeed, but
it nonetheless addresses an existent issue in PiSmmCpuDxeSmm that can be
hit during S3.

My goal is to apply that series (the first 3 patches from Jeff, and the
fourth patch from Jiewen), and to test it as one unit. I'd like to see
if those changes fix the infrequent, but still triggerable issues with
S3+SMM, for both Ia32 and Ia32X64. If everything works fine, then that
series should be committed.

After that, I'd like to test Jiewen's v4 series for the SMM page level
protection, separately.

In other words, first we should fix the existent bugs that Jiewen's SMM
page level protection feature only amplifies (but doesn't introduce) on
QEMU/KVM + OVMF. Once the known bugs are fixed, I'll be glad to test the
new feature.

Would this work for you guys?

Thank you,
Laszlo

with DEBUG message update you suggested.

My unit test failed before. Now it can pass.
I validated on a real IA32 and Windows OVMF with and without XD.


For QEMU installation, it is still on progress.
We have setup a Fedora 24 host, download QEMU, and install it.
But we are still struggling to make QEMU boot on Fedora.
Your step by step is great. There is still some minor place we stuck in due to my ignorance.
My goal is still to setup an environment like yours for our validation or issue reproduce.
It just need take some time, more than I expected. sign...

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@...] On Behalf Of
Jiewen Yao
Sent: Friday, November 11, 2016 5:01 PM
To: edk2-devel@...
Cc: Tian, Feng <feng.tian@...>; Kinney, Michael D
<michael.d.kinney@...>; Paolo Bonzini <pbonzini@...>;
Laszlo Ersek <lersek@...>; Fan, Jeff <jeff.fan@...>; Zeng,
Star <star.zeng@...>
Subject: [edk2] [PATCH V3 0/6] Enable SMM page level protection.


==== below is V3 description ====
1) PiSmmCpu: Fix CpuIndex corruption issue due to stack malposition.
(Many thanks to Laszlo Ersek <lersek@...> for catching it.)
2) PiSmmCpu: Add ASSERT for CpuIndex check.
3) PiSmmCpu: Use DEBUG_VERBOSE for page table update.
4) PiSmmCpu: Do not report DEBUG message for Ap non present
when PcdCpuSmmSyncMode==1 (Relex mode).
5) PiSmmCpu: Do not report DEBUG message for AP removed
when PcdCpuHotPlugSupport==TRUE.

Tested combination:
1) XD disabled
2) XD enabled in SMM and disabled in non-SMM.
3) XD enabled in SMM and enabled in non-SMM.

==== below is V2 description ====
1) PiSmmCpu: resolve OVMF multiple processors boot hang issue.
2) PiSmmCpu: Add debug info on StartupAp() fails.
3) PiSmmCpu: Add ASSERT for AllocatePages().
4) PiSmmCpu: Add protection detail in commit message.
5) UefiCpuPkg.dsc: Add page table footprint info in commit message.

==== below is V1 description ====
This series patch enables SMM page level protection.
Features are:
1) PiSmmCore reports SMM PE image code/data information
in EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable
and set XD for data page and RO for code page.
3) PiSmmCpu enables Static Paging for X64 according to
PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G
is used as long as it is supported.
4) PiSmmCpu sets importance data structure to be read only,
such as Gdt, Idt, SmmEntrypoint, and PageTable itself.

tested platform:
1) Intel internal platform (X64).
2) EDKII Quark IA32
3) EDKII Vlv2 X64
4) EDKII OVMF IA32 and IA32X64. (with -smp 8)

Cc: Jeff Fan <jeff.fan@...>
Cc: Feng Tian <feng.tian@...>
Cc: Star Zeng <star.zeng@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@...>

Jiewen Yao (6):
MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
QuarkPlatformPkg/dsc: enable Smm paging protection.

MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 66 +
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 1509
++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/Page.c | 775
+++++++++-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 40
+
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 91
++
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 2
+
MdeModulePkg/Core/PiSmmCore/Pool.c | 16
+
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h | 51 +
MdeModulePkg/MdeModulePkg.dec |
3 +
QuarkPlatformPkg/Quark.dsc | 6 +
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 71
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 75
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 79
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 226
+--
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 36
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 37
+-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 4
+-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 135
+-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |
144 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |
156 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |
5 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |
871 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 39
+-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 274
+++-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 59
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 62
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S | 250
+---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm | 35
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.nasm | 31
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 30
+-
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 7
+-
UefiCpuPkg/UefiCpuPkg.dec | 8 +
36 files changed, 4585 insertions(+), 803 deletions(-)
create mode 100644
MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
create mode 100644
MdeModulePkg/Include/Guid/PiSmmMemoryAttributesTable.h
create mode 100644
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

--
2.7.4.windows.1

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


Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc

Fan, Jeff <jeff.fan@...>
 

Laszlo,

Thanks your updating and pushing.

Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Friday, November 11, 2016 8:08 PM
To: Fan, Jeff; edk2-devel@...
Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen
Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc

On 11/11/16 11:51, Laszlo Ersek wrote:
On 11/11/16 09:56, Jeff Fan wrote:
Current implementation just allocates reserve memory for AsmRelocateApLoopFunc.
It not be safe because APs will be placed into 32bit protected mode
on long mode DXE. This reserve memory must be located below 4GB memory.

This fix is to allocate < 4GB memory for AsmRelocateApLoopFunc.

Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 27
++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index eb36d6f..4b929ff 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -286,7 +286,8 @@ InitMpGlobalData (
IN CPU_MP_DATA *CpuMpData
)
{
- EFI_STATUS Status;
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS Address;

SaveCpuMpData (CpuMpData);

@@ -298,16 +299,28 @@ InitMpGlobalData (
}

//
- // Avoid APs access invalid buff data which allocated by
BootServices,
- // so we will allocate reserved data for AP loop code.
+ // Avoid APs access invalid buffer data which allocated by
+ BootServices, // so we will allocate reserved data for AP loop
+ code. We also need to
There was a superfluous space character at the end of this line (reported by git-am), but I removed it.

+ // allocate this buffer below 4GB due to APs may be transferred to
+ 32bit // protected mode on long mode DXE.
// Allocating it in advance since memory services are not available in
// Exit Boot Services callback function.
//
- mReservedApLoopFunc = AllocateReservedCopyPool (
- CpuMpData->AddressMap.RelocateApLoopFuncSize,
- CpuMpData->AddressMap.RelocateApLoopFuncAddress
- );
+ Address = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiReservedMemoryType,
+ EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)),
+ &Address
+ );
+ ASSERT_EFI_ERROR (Status);
+ mReservedApLoopFunc = (VOID *) (UINTN) Address;
ASSERT (mReservedApLoopFunc != NULL);
+ CopyMem (
+ mReservedApLoopFunc,
+ CpuMpData->AddressMap.RelocateApLoopFuncAddress,
+ CpuMpData->AddressMap.RelocateApLoopFuncSize
+ );

Status = gBS->CreateEvent (
EVT_TIMER | EVT_NOTIFY_SIGNAL,
Reviewed-by: Laszlo Ersek <lersek@...>
Tested-by: Laszlo Ersek <lersek@...>
[lersek@...: strip whitespace at EOL]
Signed-off-by: Laszlo Ersek <lersek@...>

Commit ffd6b0b1b65e.

(I pushed the patch because it's simple -- Jeff is the sole maintainer, according to Maintainers.txt, of UefiCpuPkg, and I thought he'd push this simple patch with just my review anyway. I'm trying to flush my review / test queue, simplifying the possible orderings between the pending patch sets.)

Thanks
Laszlo


Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc

Laszlo Ersek
 

On 11/11/16 11:51, Laszlo Ersek wrote:
On 11/11/16 09:56, Jeff Fan wrote:
Current implementation just allocates reserve memory for AsmRelocateApLoopFunc.
It not be safe because APs will be placed into 32bit protected mode on long mode
DXE. This reserve memory must be located below 4GB memory.

This fix is to allocate < 4GB memory for AsmRelocateApLoopFunc.

Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index eb36d6f..4b929ff 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -286,7 +286,8 @@ InitMpGlobalData (
IN CPU_MP_DATA *CpuMpData
)
{
- EFI_STATUS Status;
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS Address;

SaveCpuMpData (CpuMpData);

@@ -298,16 +299,28 @@ InitMpGlobalData (
}

//
- // Avoid APs access invalid buff data which allocated by BootServices,
- // so we will allocate reserved data for AP loop code.
+ // Avoid APs access invalid buffer data which allocated by BootServices,
+ // so we will allocate reserved data for AP loop code. We also need to
There was a superfluous space character at the end of this line
(reported by git-am), but I removed it.

+ // allocate this buffer below 4GB due to APs may be transferred to 32bit
+ // protected mode on long mode DXE.
// Allocating it in advance since memory services are not available in
// Exit Boot Services callback function.
//
- mReservedApLoopFunc = AllocateReservedCopyPool (
- CpuMpData->AddressMap.RelocateApLoopFuncSize,
- CpuMpData->AddressMap.RelocateApLoopFuncAddress
- );
+ Address = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiReservedMemoryType,
+ EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)),
+ &Address
+ );
+ ASSERT_EFI_ERROR (Status);
+ mReservedApLoopFunc = (VOID *) (UINTN) Address;
ASSERT (mReservedApLoopFunc != NULL);
+ CopyMem (
+ mReservedApLoopFunc,
+ CpuMpData->AddressMap.RelocateApLoopFuncAddress,
+ CpuMpData->AddressMap.RelocateApLoopFuncSize
+ );

Status = gBS->CreateEvent (
EVT_TIMER | EVT_NOTIFY_SIGNAL,
Reviewed-by: Laszlo Ersek <lersek@...>
Tested-by: Laszlo Ersek <lersek@...>
[lersek@...: strip whitespace at EOL]
Signed-off-by: Laszlo Ersek <lersek@...>

Commit ffd6b0b1b65e.

(I pushed the patch because it's simple -- Jeff is the sole maintainer,
according to Maintainers.txt, of UefiCpuPkg, and I thought he'd push
this simple patch with just my review anyway. I'm trying to flush my
review / test queue, simplifying the possible orderings between the
pending patch sets.)

Thanks
Laszlo


[PATCH 2/2] UefiCpuPkg/DxeMpLib: Place APs to suitable state on Legacy OS boot

Jeff Fan <jeff.fan@...>
 

Currently, DxeMpLib only places APs into specified c-state in Exit Boot Service
callback function for UEFI OS boot. We need to put APs into specified c-state
for legacy OS boot also.

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

Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Cc: Feng Tian <feng.tian@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 12 ++++++++++++
2 files changed, 13 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 03a8994..972c9ad 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -58,6 +58,7 @@

[Guids]
gEfiEventExitBootServicesGuid ## CONSUMES ## Event
+ gEfiEventLegacyBootGuid ## CONSUMES ## Event

[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7ba4b80..793d947 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -22,6 +22,7 @@
CPU_MP_DATA *mCpuMpData = NULL;
EFI_EVENT mCheckAllApsEvent = NULL;
EFI_EVENT mMpInitExitBootServicesEvent = NULL;
+EFI_EVENT mLegacyBootEvent = NULL;
volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
VOID *mReservedApLoopFunc = NULL;

@@ -340,6 +341,7 @@ InitMpGlobalData (
AP_CHECK_INTERVAL
);
ASSERT_EFI_ERROR (Status);
+
Status = gBS->CreateEvent (
EVT_SIGNAL_EXIT_BOOT_SERVICES,
TPL_CALLBACK,
@@ -348,6 +350,16 @@ InitMpGlobalData (
&mMpInitExitBootServicesEvent
);
ASSERT_EFI_ERROR (Status);
+
+ Status = gBS->CreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_CALLBACK,
+ MpInitChangeApLoopCallback,
+ NULL,
+ &gEfiEventLegacyBootGuid,
+ &mLegacyBootEvent
+ );
+ ASSERT_EFI_ERROR (Status);
}

/**
--
2.9.3.windows.2


[PATCH 1/2] UefiCpuPkg/DxeMpLib: Rename MpInitExitBootServicesCallback()

Jeff Fan <jeff.fan@...>
 

Rename MpInitExitBootServicesCallback() to MpInitChangeApLoopCallback() because
it will not only be invoked on Exit Boot Service Event, but also will be invoked
on Legacy Ready To Boot Event.

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

Cc: Laszlo Ersek <lersek@...>
Cc: Paolo Bonzini <pbonzini@...>
Cc: Feng Tian <feng.tian@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 4b929ff..7ba4b80 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -261,7 +261,7 @@ RelocateApLoop (
**/
VOID
EFIAPI
-MpInitExitBootServicesCallback (
+MpInitChangeApLoopCallback (
IN EFI_EVENT Event,
IN VOID *Context
)
@@ -273,7 +273,7 @@ MpInitExitBootServicesCallback (
CpuMpData->PmCodeSegment = GetProtectedModeCS ();
CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, mReservedApLoopFunc);
- DEBUG ((DEBUG_INFO, "MpInitExitBootServicesCallback() done!\n"));
+ DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
}

/**
@@ -343,7 +343,7 @@ InitMpGlobalData (
Status = gBS->CreateEvent (
EVT_SIGNAL_EXIT_BOOT_SERVICES,
TPL_CALLBACK,
- MpInitExitBootServicesCallback,
+ MpInitChangeApLoopCallback,
NULL,
&mMpInitExitBootServicesEvent
);
--
2.9.3.windows.2


[PATCH 0/2] Place APs to suitable state on Legacy OS boot

Jeff Fan <jeff.fan@...>
 

Currently, DxeMpLib only places APs into specified c-state in Exit Boot Service
callback function for UEFI OS boot. We need to put APs into specified c-state
for legacy OS boot also.

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

Jeff Fan (2):
UefiCpuPkg/DxeMpLib: Rename MpInitExitBootServicesCallback()
UefiCpuPkg/DxeMpLib: Place APs to suitable state on Legacy OS boot

UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 18 +++++++++++++++---
2 files changed, 16 insertions(+), 3 deletions(-)

--
2.9.3.windows.2


Re: [PATCH v2 6/9] OvmfPkg/PlatformBds: Dispatch deferred images after EndOfDxe

Ni, Ruiyu <ruiyu.ni@...>
 

Sorry for that! :(
I really forgot it.

发自我的 iPhone

在 2016年11月11日,下午7:16,Laszlo Ersek <lersek@...> 写道:

Ray,

On 11/08/16 14:04, Laszlo Ersek wrote:
On 11/08/16 13:29, Ruiyu Ni wrote:
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
---
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 66ee590..cc35630 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -389,6 +389,11 @@ Returns:
NULL);
ASSERT_EFI_ERROR (Status);

+ //
+ // Dispatch deferred images after EndOfDxe event and ReadyToLock installation.
+ //
+ EfiBootManagerDispatchDeferredImages ();
+
PlatformInitializeConsole (gPlatformConsole);
PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
GetFrontPageTimeoutFromQemu ());
Can you please add a note to the commit message that, if the platform
installs EfiDxeSmmReadyToLockProtocol, then the deferred images must not
be dispatched prior to that either, not just prior to EndOfDxe?

Simultaneously, I propose the following subject (72 chars):

OvmfPkg/PlatformBds: Dispatch deferred images after EndOfDxe/ReadyToLock

No need to repost just because of this; the commit message can be
updated before you commit the series.

With the commit message update:

Reviewed-by: Laszlo Ersek <lersek@...>
I see that you added my R-b to the commit message.

9789894e3ba3dae87bfc384e97f946caeab389e0

But, you didn't update the patch like I requested. :(

Please pay more attention. Otherwise next time I'll have to ask for a
full repost, to make sure that my request is addressed.

Laszlo

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


Re: File mode problem on Github edk2-BaseTools-win32

Laszlo Ersek
 

On 11/11/16 12:05, Evan Lloyd wrote:
There is a minor, but annoying, problem with file modes on the Github edk2-BaseTools-win32 repository.
Git maintains a limited internal record of the Unix style file modes.
edk2-BaseTools-win32 currently causes Git to set the file's modes to 660.
So, after a checkout or pull of master, the builds fail because the files do not have Windows' "Read & Execute" permission.
This is simple to fix, but one has to remember (or, ofttimes, get reminded) to do it every time there is an update.

Liming,
Because this is purely a permission problem in the Git repository, and .exe files are not amenable to patching,
They are -- I think if you change the file mode bits, git will see that, and will create a patch that has no content hunks, just the file mode changes.

For example, in the BaseTools/Conf/ directory, we happen have two template files that have gratuitous execute permissions. If I remove those permissions, "git diff" shows

diff --git a/BaseTools/Conf/build_rule.template b/BaseTools/Conf/build_rule.template
old mode 100755
new mode 100644
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
old mode 100755
new mode 100644
The same should be possible on Windows too. (You just need the inverse operation right now, chmod +x.)

(The above patch is one I could submit genuinely, but I'm too lazy. :))

I have raised a pull request on https://github.com/tianocore/edk2-BaseTools-win32/pulls
This is only a minor thing, but I would deem it a great favour were you to accept the pull request.
It has me tearing my hair out, and I have little enough to begin with. :-{
It is fine to send pull requests, but:
- they should be mailed to the list (not opened on github),
- the patches have to be reviewed first, anyway.

(Speaking about the edk2 repo at least -- I realize this is a different repo.)

Thanks
Laszlo