[edk2] [Patch] UefiCpuPkg/CpuS3DataDxe: Add more detailed description on GUID in INF

[Jeff Fan]

Add the GUID gEfiEndOfDxeEventGroupGuid usage description in more details in
INF file.

Cc: Feng Tian 
Cc: Michael Kinney 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan 
---
 UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf 
b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index 2ef16a7..9143b87 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -51,7 +51,7 @@
   MtrrLib
 
 [Guids]
-  gEfiEndOfDxeEventGroupGuid ## CONSUMES
+  gEfiEndOfDxeEventGroupGuid ## CONSUMES   ## Event
 
 [Protocols]
   gEfiMpServiceProtocolGuid  ## CONSUMES
-- 
1.9.5.msysgit.0

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

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Ard Biesheuvel]

On 1 December 2015 at 01:24, Vladimir Olovyannikov
 wrote:
> Thank you Andrew, Ard.
>
> The problem was my wrong interpretation of the offset in DS-5 for the 
> ShellPkg,
> and therefore wrong entry point arithmetic.
>

OK, thanks for clearing that up.

Regards,
Ard.


>> -Original Message-
>> From: af...@apple.com [mailto:af...@apple.com]
>> Sent: Monday, November 30, 2015 10:02 AM
>> To: Vladimir Olovyannikov
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with
>> step-by-step debugging in uefi
>>
>>
>> > On Nov 23, 2015, at 11:31 PM, Ard Biesheuvel 
>> wrote:
>> >
>> > On 24 November 2015 at 00:38, Vladimir Olovyannikov
>> >  wrote:
>> >>
>> >>
>> >>> -Original Message-
>> >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> >>> Sent: Tuesday, November 10, 2015 10:04 AM
>> >>> To: Vladimir Olovyannikov
>> >>> Cc: Cohen, Eugene; edk2-devel@lists.01.org
>> >>> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
>> with
>> >>> step-by-step debugging in uefi
>> >>>
>> >>> On 10 November 2015 at 18:41, Vladimir Olovyannikov
>> >>>  wrote:
>>  Ard,
>>  Many thanks for your help. It works.
>> 
>> >>>
>> >>> Great! Thanks for reporting back.
>> >>>
>> >>>
>>  -Original Message-
>>  From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>  Sent: Monday, November 09, 2015 10:31 PM
>>  To: Vladimir Olovyannikov
>>  Cc: Cohen, Eugene; edk2-devel@lists.01.org
>>  Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
>> >>> with step-by-step debugging in uefi
>> 
>>  On 9 November 2015 at 19:01, Vladimir Olovyannikov
>>   wrote:
>> > -Original Message-
>> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> > Sent: Sunday, November 08, 2015 10:52 PM
>> > To: Vladimir Olovyannikov
>> > Cc: Cohen, Eugene; edk2-devel@lists.01.org
>> > Subject: Re: [edk2] Strange behavior of the DS-5 debugger on
>> AARCH64
>> >>> with step-by-step debugging in uefi
>> >
>> > On 6 November 2015 at 21:32, Vladimir Olovyannikov
>> > > wrote:
>> >>> Hello Ard, Eugene,
>> >>> Thank you for explanation.
>> >>>
>> >>> Ard, I tried the patch, but it cannot be applied to the latest 
>> >>> (pulled a
>> >>> minute ago, git-svn-id:
>> >>> https://svn.code.sf.net/p/edk2/code/trunk/edk2@18732 6f19259b-
>> 4bc3-
>> >>> 4df7-8a09-765794883524)
>> >>> tree: all 3 hunks failed. Which commit should I be based on to apply
>> the
>> >>> patch?
>> >>>
>> >>> Anyway I found the lines manually and changed them. However,
>> when
>> >>> I try to
>> >>>
>> >>> source /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
>> >>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>> >>> I am getting
>> >>>
>> >>> ERROR(?): ValueError: need more than 1 value to unpack
>> >>>  File " /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py",
>> line
>> >>> 94, in >
>> >>>armplatform_debugger.load_all_symbols()
>> >>> ERROR(CMD656):
>> >>> # in
>> /uefi/BroadcomPlatformPkg/NS2Pkg/Scripts/armpkg_syms.ds:2
>> >>> while executing: source
>> >>> /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
>> >>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>> >>> ! The script
>> /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py
>> >>> failed to complete due to an error during execution of the script
>> >>>
>> >> [...]
>> >> Ard, I got a pretty much the same issue when I tried to do some
>> debugging in the ShellPkg.
>> >> Except Shell I can perfectly debug everything.
>> >>
>> >> 1. source / uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>> >> loads symbols fine, but does not recognize any module matching the
>> current PC if stopped in the shell.
>> >> 2. Loading symbols with "add-symbol-file
>> /uefi/Build/NS2Pkg/DEBUG_GCC49/AARCH64/ShellPkg/Application/Shell/Sh
>> ell/DEBUG/Shell.dll 0xB6926000"
>> >>"recognizes" modules (wrong ones though) but the source code does
>> not match disassembly.
>> >>
>> >> So with Shell debug using DS-5 the code does not match the source.
>> >> Is there a special linker setting I am missing or a technique?
>> >> I am using the latest UEFI code from
>> >> https://github.com/tianocore/edk2.git
>> >>
>> >
>> > I am sorry, but since I don't have access to DS-5, I am not sure how
>> > to debug this.
>> >
>> > Is there any way for you to figure how much the offset is between the
>> > current and the correct location? I.e., by looking at the ELF asm dump
>> > and the instructions around the PC? Something in the order of 0x260
>> > perhaps?
>> >
>>
>> The map file in the build output directory is useful for tracking down these
>> kind of issues.
>>
>> Issues like this usually relate to the fact that PE/COFF images load the
>> PE/COFF header into memory when the code is lo

[edk2] [Patch] [patch] Vlv2TbltDevicePkg: Add SsdtUpdate Application to update Ssdt table to ACPI table.

[lushifex]

Signed-off-by: lushifex 
---
 .../Application/SsdtUpdate/SsdtUpdate.asl  |  28 +++
 .../Application/SsdtUpdate/SsdtUpdate.c| 200 +
 .../Application/SsdtUpdate/SsdtUpdate.h|  39 
 .../Application/SsdtUpdate/SsdtUpdate.inf  |  64 +++
 Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc|   1 +
 Vlv2TbltDevicePkg/PlatformPkgIA32.dsc  |   1 +
 Vlv2TbltDevicePkg/PlatformPkgX64.dsc   |   1 +
 7 files changed, 334 insertions(+)
 create mode 100644 Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.asl
 create mode 100644 Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.c
 create mode 100644 Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.h
 create mode 100644 Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.inf

diff --git a/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.asl 
b/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.asl
new file mode 100644
index 000..bdf48fa
--- /dev/null
+++ b/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.asl
@@ -0,0 +1,28 @@
+/** @file
+  The definition block in ACPI table for Genernal device.
+
+  Copyright (c) 2015, 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 
+  which accompanies this distribution.  The full text of the license may be 
found at 
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+DefinitionBlock (
+  "Gene.aml",
+  "SSDT",
+   2,
+  "INTEL ",
+  "GeneTabl",
+  0x1000
+  )
+{
+  Scope (\_SB)
+  {
+  }
+}
diff --git a/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.c 
b/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.c
new file mode 100644
index 000..f50a76d
--- /dev/null
+++ b/Vlv2TbltDevicePkg/Application/SsdtUpdate/SsdtUpdate.c
@@ -0,0 +1,200 @@
+/** @file
+  Update SSDT table to ACPI table.
+
+  Copyright (c) 2013 - 2015, 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 
+  which accompanies this distribution.  The full text of the license may be 
found at 
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "SsdtUpdate.h"
+
+FV_INPUT_DATA mInputData = {0};
+
+/**
+  Read file data from given file name.
+
+  @param[in]  FileNamePointer the readed given file name.
+  @param[out] Buffer  The buffer which read the given file name's data.
+  @param[out] BufferSize  The buffer size which read the given file name's 
data.
+
+  @retval EFI_SUCCESS The file data is successfully readed.
+  @retval EFI_ERROR   The file data is unsuccessfully readed.
+
+**/
+STATIC
+EFI_STATUS
+ReadFileData (
+  IN  CHAR16   *FileName,
+  OUT UINT8**Buffer,
+  OUT UINT32   *BufferSize
+  )
+{
+  EFI_STATUS Status;
+  SHELL_FILE_HANDLE  FileHandle;
+  UINT64 Size;
+  VOID   *NewBuffer;
+  UINTN  ReadSize;
+
+  FileHandle = NULL;
+  NewBuffer = NULL;
+  Size = 0;
+
+  Status = ShellOpenFileByName (FileName, &FileHandle, EFI_FILE_MODE_READ, 0);
+  if (EFI_ERROR (Status)) {
+goto Done;
+  }
+
+  Status = FileHandleIsDirectory (FileHandle);
+  if (!EFI_ERROR (Status)) {
+Status = EFI_NOT_FOUND;
+goto Done;
+  }
+
+  Status = FileHandleGetSize (FileHandle, &Size);
+  if (EFI_ERROR (Status)) {
+goto Done;
+  }
+
+  NewBuffer = AllocatePool ((UINTN) Size);
+
+  ReadSize = (UINTN) Size;
+  Status = FileHandleRead (FileHandle, &ReadSize, NewBuffer);
+  if (EFI_ERROR (Status)) {
+goto Done;
+  } else if (ReadSize != (UINTN) Size) {
+Status = EFI_INVALID_PARAMETER;
+goto Done;
+  }
+
+Done:
+  if (FileHandle != NULL) {
+ShellCloseFile (&FileHandle);
+  }
+
+  if (EFI_ERROR (Status)) {
+if (NewBuffer != NULL) {
+  FreePool (NewBuffer);
+}
+  } else {
+*Buffer = NewBuffer;
+*BufferSize = (UINT32) Size;
+  }
+
+  return Status;
+}
+
+/**
+  Initialize and publish device in ACPI table.
+
+  @param[in] Table  The pointer to the ACPI table which will be 
published. 
+  @param[in] TableSize  The size of ACPI table which will be published.
+
+  @retval   EFI_SUCCESS The ACPI table is published successfully.
+  @retval   Others  The ACPI table is not published.
+
+**/
+EFI_STATUS
+PublishAcpiTable (
+  IN UINT8   *Table,
+  IN UINT32  TableSize
+  )
+{
+  EFI_STATUS Status;
+  EFI_ACPI_TABLE_PROTOCOL*AcpiTable;
+  UINTN  TableKey;
+
+  //
+  // Basic check ::TODO: Add check here

Re: [edk2] [Patch] UefiCpuPkg/CpuMpPei: Fix typo and add some comments

[Tian, Feng]

Reviewed-by: Feng Tian 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jeff Fan
Sent: Thursday, November 26, 2015 13:27
To: edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Tian, Feng
Subject: [edk2] [Patch] UefiCpuPkg/CpuMpPei: Fix typo and add some comments

Cc: Feng Tian 
Cc: Michael Kinney 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c 
index ca48613..ab1260d 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -212,7 +212,7 @@ RestoreVolatileRegisters (
   This function will be called from AP reset code if BSP uses WakeUpAP.
 
   @param ExchangeInfo Pointer to the MP exchange info buffer
-  @param NumApsExecuting  Number of curret executing AP
+  @param NumApsExecuting  Number of current executing AP
 **/
 VOID
 EFIAPI
@@ -268,6 +268,9 @@ ApCFunction (
 (PeiCpuMpData->ApFunction != 0)) {
   PeiCpuMpData->CpuData[ProcessorNumber].State = CpuStateBusy;
   Procedure = (EFI_AP_PROCEDURE)(UINTN)PeiCpuMpData->ApFunction;
+  //
+  // Invoke AP function here
+  //
   Procedure ((VOID *)(UINTN)PeiCpuMpData->ApFunctionArgument);
   PeiCpuMpData->CpuData[ProcessorNumber].State = CpuStateIdle;
 }
@@ -612,7 +615,7 @@ CpuMpEndOfPeiCallback (
   EFI_PEI_HOB_POINTERS  Hob;
   EFI_HOB_MEMORY_ALLOCATION *MemoryHob;
 
-  DEBUG ((EFI_D_INFO, "CpuMpPei: CpuMpEndOfPeiCallback () invokded\n"));
+  DEBUG ((EFI_D_INFO, "CpuMpPei: CpuMpEndOfPeiCallback () invoked\n"));
 
   Status = PeiServicesGetBootMode (&BootMode);
   ASSERT_EFI_ERROR (Status);
--
1.9.5.msysgit.0

___
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] UefiCpuPkg/CpuMpPei: Sync/Save/Restore CRx/DRx register

[Tian, Feng]

Looks good to me.

Reviewed-by: Feng Tian 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jeff Fan
Sent: Thursday, November 26, 2015 13:24
To: edk2-de...@ml01.01.org
Subject: [edk2] [Patch 0/5] UefiCpuPkg/CpuMpPei: Sync/Save/Restore CRx/DRx 
register

This serial of patches will sync BSP's CRx to APs when AP initialization.

PeiStartupAllAPs()/PeiStartupThisAP() will send INIT-SIPI-SIPI to wakeup APs to 
execute AP function. However, some registers will be reset after APs received 
INIT IPI. We need to restore some registers (For example, CRx/DRx) manually 
after APs wakeup.

Jeff Fan (5):
  UefiCpuPkg/CpuMpPei: Exchange whole CPU data in SortApicId()
  UefiCpuPkg/CpuMpPei: Add CPU_VOLATILE_REGISTERS & worker functions
  UefiCpuPkg/CpuMpPei: Set AP state to CpuStateIdle after initialization
  UefiCpuPkg/CpuMpPei: Sync BSP's CRx to APs when initialization
  UefiCpuPkg/CpuMpPei: Save/Restore CRx/DRx register for APs waking up

 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 106 +
 UefiCpuPkg/CpuMpPei/CpuMpPei.h |  13 +
 2 files changed, 109 insertions(+), 10 deletions(-)

--
1.9.5.msysgit.0

___
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] BaseTools: Add support for INF statement in FD region

[Yonghong Zhu]

FD region today can be file or data, but not a patched image.Add support
for an INF statement in an FD region, so the binary from the INF can be
patched prior to being added to the FD region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 Source/Python/GenFds/FdfParser.py   | 57 +
 Source/Python/GenFds/FfsInfStatement.py | 44 +++--
 Source/Python/GenFds/Region.py  | 21 
 3 files changed, 91 insertions(+), 31 deletions(-)

diff --git a/Source/Python/GenFds/FdfParser.py 
b/Source/Python/GenFds/FdfParser.py
index 664bf8e..01c035c 100644
--- a/Source/Python/GenFds/FdfParser.py
+++ b/Source/Python/GenFds/FdfParser.py
@@ -1,9 +1,9 @@
 ## @file
 # parse FDF file
 #
-#  Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.
+#  Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.
 #  Copyright (c) 2015, Hewlett Packard Enterprise Development, L.P.
 #
 #  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
@@ -1844,11 +1844,11 @@ class FdfParser:
 RegionObj.Size = Size
 
 if not self.__GetNextWord():
 return True
 
-if not self.__Token in ("SET", "FV", "FILE", "DATA", "CAPSULE"):
+if not self.__Token in ("SET", "FV", "FILE", "DATA", "CAPSULE", "INF"):
 #
 # If next token is a word which is not a valid FV type, it might 
be part of [PcdOffset[|PcdSize]]
 # Or it might be next region's offset described by an expression 
which starts with a PCD.
 #PcdOffset[|PcdSize] or OffsetPcdExpression|Size
 #
@@ -1885,21 +1885,31 @@ class FdfParser:
 self.__UndoToken()
 self.__GetRegionCapType( RegionObj)
 
 elif self.__Token == "FILE":
 self.__UndoToken()
-self.__GetRegionFileType( RegionObj)
+self.__GetRegionFileType(RegionObj)
+
+elif self.__Token == "INF":
+self.__UndoToken()
+RegionObj.RegionType = "INF"
+while self.__IsKeyword("INF"):
+self.__UndoToken()
+ffsInf = self.__ParseInfStatement()
+if not ffsInf:
+break
+RegionObj.RegionDataList.append(ffsInf)
 
 elif self.__Token == "DATA":
 self.__UndoToken()
-self.__GetRegionDataType( RegionObj)
+self.__GetRegionDataType(RegionObj)
 else:
 self.__UndoToken()
 if self.__GetRegionLayout(Fd):
 return True
 raise Warning("A valid region type was not found. "
-  "Valid types are [SET, FV, CAPSULE, FILE, DATA]. 
This error occurred",
+  "Valid types are [SET, FV, CAPSULE, FILE, DATA, 
INF]. This error occurred",
   self.FileName, self.CurrentLineNumber)
 
 return True
 
 ## __GetRegionFvType() method
@@ -2424,27 +2434,16 @@ class FdfParser:
 raise Warning("expected '}'", self.FileName, 
self.CurrentLineNumber)
 
 FvObj.AprioriSectionList.append(AprSectionObj)
 return True
 
-## __GetInfStatement() method
-#
-#   Get INF statements
-#
-#   @param  selfThe object pointer
-#   @param  Obj for whom inf statement is got
-#   @param  MacroDict   dictionary used to replace macro
-#   @retval TrueSuccessfully find inf statement
-#   @retval False   Not able to find inf statement
-#
-def __GetInfStatement(self, Obj, ForCapsule = False, MacroDict = {}):
-
-if not self.__IsKeyword( "INF"):
-return False
+def __ParseInfStatement(self):
+if not self.__IsKeyword("INF"):
+return None
 
 ffsInf = FfsInfStatement.FfsInfStatement()
-self.__GetInfOptions( ffsInf)
+self.__GetInfOptions(ffsInf)
 
 if not self.__GetNextToken():
 raise Warning("expected INF file path", self.FileName, 
self.CurrentLineNumber)
 ffsInf.InfFileName = self.__Token
 
@@ -2470,11 +2469,27 @@ class FdfParser:
 ffsInf.KeepReloc = False
 elif self.__IsKeyword('RELOCS_RETAINED'):
 ffsInf.KeepReloc = True
 else:
 raise Warning("Unknown reloc strip flag '%s'" % self.__Token, 
self.FileName, self.CurrentLineNumber)
-
+return ffsInf
+
+## __GetInfStatement() method
+#
+#   Get INF statements
+#
+#   @param  selfThe object pointer
+#   @param  Obj for whom inf statement is got
+#   @param  MacroDict   dictionary used to replace macro
+#   @retval TrueSuccessfully find inf statement
+#   @retval False   Not a

[edk2] [Patch] BaseTools: Fix a bug when apply patches to SEC use the FILE_GUID override

[Yonghong Zhu]

Fix a bug when applying patches to SEC modules that use the FILE_GUID
override. Since a temp dir is used when FILE_GUID override is used, the
INF file path comparisons fail. The fix is to capture the real INF file
path comparisons instead of using the temp dir path to the INF.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 Source/Python/GenFds/FfsInfStatement.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Source/Python/GenFds/FfsInfStatement.py 
b/Source/Python/GenFds/FfsInfStatement.py
index b9cb4f2..6d47a7a 100644
--- a/Source/Python/GenFds/FfsInfStatement.py
+++ b/Source/Python/GenFds/FfsInfStatement.py
@@ -172,10 +172,14 @@ class FfsInfStatement(FfsInfStatementClassObject):
 PathClassObj = PathClass(self.InfFileName, 
GenFdsGlobalVariable.WorkSpaceDir)
 ErrorCode, ErrorInfo = PathClassObj.Validate(".inf")
 if ErrorCode != 0:
 EdkLogger.error("GenFds", ErrorCode, ExtraData=ErrorInfo)
 
+#
+# Cache lower case version of INF path before processing FILE_GUID 
override
+#
+InfLowerPath = str(PathClassObj).lower()
 if self.OverrideGuid:
 PathClassObj = ProcessDuplicatedInf(PathClassObj, 
self.OverrideGuid, GenFdsGlobalVariable.WorkSpaceDir)
 if self.CurrentArch != None:
 
 Inf = GenFdsGlobalVariable.WorkSpace.BuildObject[PathClassObj, 
self.CurrentArch, GenFdsGlobalVariable.TargetName, 
GenFdsGlobalVariable.ToolChainTag]
@@ -239,11 +243,10 @@ class FfsInfStatement(FfsInfStatementClassObject):
 continue
 if Pcd.Type != 'PatchableInModule':
 continue
 # Override Patchable PCD value by the value from DSC
 PatchPcd = None
-InfLowerPath = str(PathClassObj).lower()
 if InfLowerPath in DscModules and PcdKey in 
DscModules[InfLowerPath].Pcds:
 PatchPcd = DscModules[InfLowerPath].Pcds[PcdKey]
 elif PcdKey in Platform.Pcds:
 PatchPcd = Platform.Pcds[PcdKey]
 DscOverride = False
-- 
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 3/5] SecurityPkg: Enable Customized Secure Boot feature

[Zeng, Star]

On 2015/11/3 15:34, Zhang, Chao B wrote:

Enable Secure Boot feature defined in UEFI2.5 ECR1263. Add
gEfiSecureBootModeGuid definition

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
  SecurityPkg/Include/Guid/AuthenticatedVariableFormat.h | 1 +
  SecurityPkg/SecurityPkg.dec| 6 +-
  2 files changed, 6 insertions(+), 1 deletion(-)


The code change is good to me.
And please follow Laszlo's suggestion in [PATCH 0/5] to update the patch 
title.


If you follow the suggestion above, you can have my
Reviewed-by: Star Zeng 

Thanks,
Star



diff --git a/SecurityPkg/Include/Guid/AuthenticatedVariableFormat.h 
b/SecurityPkg/Include/Guid/AuthenticatedVariableFormat.h
index 1f007cf..dfd3f36 100644
--- a/SecurityPkg/Include/Guid/AuthenticatedVariableFormat.h
+++ b/SecurityPkg/Include/Guid/AuthenticatedVariableFormat.h
@@ -28,6 +28,7 @@ extern EFI_GUID gEfiSecureBootEnableDisableGuid;
  extern EFI_GUID gEfiCertDbGuid;
  extern EFI_GUID gEfiCustomModeEnableGuid;
  extern EFI_GUID gEfiVendorKeysNvGuid;
+extern EFI_GUID gEfiSecureBootModeGuid;

  ///
  /// "SecureBootEnable" variable for the Secure Boot feature enable/disable.
diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 959acf0..f768161 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -109,7 +109,11 @@
## GUID used to "certdb" variable to store the signer's certificates for 
common variables with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute.
#  Include/Guid/AuthenticatedVariableFormat.h
gEfiCertDbGuid = { 0xd9bee56e, 0x75dc, 0x49d9, { 0xb4, 
0xd7, 0xb5, 0x34, 0x21, 0xf, 0x63, 0x7a } }
-
+
+  ## GUID used to "SecureBootMode" variable to save platform secure boot mode
+  #  Include/Guid/AuthenticatedVariableFormat.h
+  gEfiSecureBootModeGuid = { 0xc573b77, 0xeb93, 0x4d3d, { 0xaf, 
0xfc, 0x5f, 0xeb, 0xca, 0xfb, 0x65, 0xb0 } }
+
## Hob GUID used to pass a TCG_PCR_EVENT from a TPM PEIM to a TPM DXE 
Driver.
#  Include/Guid/TcgEventHob.h
gTcgEventEntryHobGuid  = { 0x2b9ffb52, 0x1b13, 0x416f, { 0xa8, 
0x7b, 0xbc, 0x93, 0xd, 0xef, 0x92, 0xa8 }}



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

Re: [edk2] [PATCH 2/5] SecurityPkg: Enable Customized Secure Boot feature

[Zeng, Star]

On 2015/11/3 15:34, Zhang, Chao B wrote:

Enable Secure Boot feature defined in UEFI2.5 ECR1263. Add 
AuditMode/DeployedMode value definition.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
  MdePkg/Include/Guid/GlobalVariable.h  | 14 ++
  MdePkg/Include/Guid/ImageAuthentication.h |  9 ++---
  2 files changed, 20 insertions(+), 3 deletions(-)


The code change is good to me.
And please follow Laszlo's suggestion in [PATCH 0/5] to update the patch 
title.
Another, please put this patch to be before[PATCH 1/5] since this patch 
is to add the definitions, and other patches are to consume the definitions.


If you follow the suggestions above, you can have my
Reviewed-by: Star Zeng 

Thanks,
Star



diff --git a/MdePkg/Include/Guid/GlobalVariable.h 
b/MdePkg/Include/Guid/GlobalVariable.h
index 1e4fbc8..e58f7a1 100644
--- a/MdePkg/Include/Guid/GlobalVariable.h
+++ b/MdePkg/Include/Guid/GlobalVariable.h
@@ -126,6 +126,20 @@ extern EFI_GUID gEfiGlobalVariableGuid;
  ///
  #define EFI_SETUP_MODE_NAME L"SetupMode"
  ///
+/// Whether the system is operating in audit mode (1) or not (0).
+/// All other values are reserved. Should be treated as read-only except when 
DeployedMode is 0.
+/// Always becomes read-only after ExitBootServices() is called.
+/// Its attribute is BS+RT.
+///
+#define EFI_AUDIT_MODE_NAME L"AuditMode"
+///
+/// Whether the system is operating in deployed mode (1) or not (0).
+/// All other values are reserved. Should be treated as read-only when its 
value is 1.
+/// Always becomes read-only after ExitBootServices() is called.
+/// Its attribute is BS+RT.
+///
+#define EFI_DEPLOYED_MODE_NAME  L"DeployedMode"
+///
  /// The Key Exchange Key Signature Database.
  /// Its attribute is NV+BS+RT+AT.
  ///
diff --git a/MdePkg/Include/Guid/ImageAuthentication.h 
b/MdePkg/Include/Guid/ImageAuthentication.h
index 4f42960..2f51935 100644
--- a/MdePkg/Include/Guid/ImageAuthentication.h
+++ b/MdePkg/Include/Guid/ImageAuthentication.h
@@ -43,9 +43,12 @@

  #define SECURE_BOOT_MODE_ENABLE   1
  #define SECURE_BOOT_MODE_DISABLE  0
-#define SETUP_MODE1
-#define USER_MODE 0
-
+#define SETUP_MODE_ENABLE 1
+#define SETUP_MODE_DISABLE0
+#define DEPLOYED_MODE_ENABLE  1
+#define DEPLOYED_MODE_DISABLE 0
+#define AUDIT_MODE_ENABLE 1
+#define AUDIT_MODE_DISABLE0

  //***
  // Signature Database



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

Re: [edk2] [PATCH 1/5] SecurityPkg: Enable Customized Secure Boot feature

[Zeng, Star]

On 2015/11/3 15:34, Zhang, Chao B wrote:

Enable Secure Boot feature defined in UEFI2.5 ECR1263. Add VarCheck for 
AuditMode/DeployedMode

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
  .../VarCheckUefiLib/VarCheckUefiLibNullClass.c | 22 ++
  1 file changed, 22 insertions(+)


The code change is good to me.
And please follow Laszlo's suggestion in [PATCH 0/5] to update the patch 
title.
Another, please put this patch to be after [PATCH 2/5] since [PATCH 2/5] 
is to add the definitions, and this patch and other following patches 
are to consume the definitions.


If you follow the suggestions above, you can have my
Reviewed-by: Star Zeng 

Thanks,
Star



diff --git a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c 
b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c
index 15144bd..a4268ae 100644
--- a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c
+++ b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c
@@ -426,6 +426,28 @@ UEFI_DEFINED_VARIABLE_ENTRY mGlobalVariableList[] = {
  NULL
},
{
+EFI_AUDIT_MODE_NAME,
+{
+  VAR_CHECK_VARIABLE_PROPERTY_REVISION,
+  0,
+  VARIABLE_ATTRIBUTE_BS_RT,
+  sizeof (UINT8),
+  sizeof (UINT8)
+},
+NULL
+  },
+  {
+EFI_DEPLOYED_MODE_NAME,
+{
+  VAR_CHECK_VARIABLE_PROPERTY_REVISION,
+  0,
+  VARIABLE_ATTRIBUTE_BS_RT,
+  sizeof (UINT8),
+  sizeof (UINT8)
+},
+NULL
+  },
+  {
  EFI_KEY_EXCHANGE_KEY_NAME,
  {
VAR_CHECK_VARIABLE_PROPERTY_REVISION,



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

[edk2] [EDK2] Why apic timer vector is 5 in Ovmf Sec?

[winddy]

Dear Expert,
I see in file OvmfPkg\Sec\SecMain.c, line 810:
  //
  // Initialize Local APIC Timer hardware and disable Local APIC Timer
  // interrupts before initializing the Debug Agent and the debug timer is
  // enabled.
  //
  InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
  DisableApicTimerInterrupt ();
  
Ovmf initializes apic timer and give it a vector number 5, according to "Intel® 
64 and IA-32 Architectures Software Developer’s Manual", vol 3, 10.5.2 Valid 
Interrupt Vectors:


When an interrupt vector in the range of 0 to 15 is sent or received through 
the local APIC, the APIC indicates an 
illegal vector in its Error Status Register (see Section 10.5.3, “Error 
Handling”). The Intel 64 and IA-32 architec-
tures reserve vectors 16 through 31 for predefined interrupts, exceptions, and 
Intel-reserved encodings (see Table 
6-1). However, the local APIC does not treat vectors in this range as illegal. 
When an illegal vector value (0 to 15) 
is written to an LVT entry and the delivery mode is Fixed (bits 8-11 equal 0), 
the APIC may signal an illegal vector 
error, without regard to whether the mask bit is set or whether an interrupt is 
actually seen on the input.



So here "vector 5" is a fake value, right? we just want to let timer be 
running, and we can fill it with 6, 7, 8, also?
If next function "DisableApicTimerInterrupt()" does not invoke, a interrupt 
will be occurred when current timer value 
down to Zero?  


And I see CPU Interrupt 5 is "BOUND Range Exceeded Exception" for BOUND 
instruction. 


Thanks.





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

Re: [edk2] [PATCH v2 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

On 2015/12/1 2:12, Laszlo Ersek wrote:
> Thank you -- this looks better. Unfortunately the
> SetMemorySpaceAttributes() question still has to be addressed, and we're
> still discussing with Ard which way to go about that. Once we have an
> agreement, I'll spell out the further requirements for this.
> 
> The "do it all in PEI with HOBs" idea is out, now we're trying to decide
> if (a) we should do it in VirtFdtDxe, and update the APRIORI DXE block
> in the FDF file so that CpuDxe gets dispatched before VirtFdtDxe, or (b)
> if we should move this code to a separate DXE driver that depends on the
> CPU architectural protocol installed by CpuDxe. Whatever the agreement,
> this patch will need to be further updated.
> 
> I ask for your patience with this -- it's messier than I would like. I
> hope your NUMA development can proceed even until we converge with the
> help of these WIP patches.
> 
It's fine since it doesn't block the NUMA patches. :)

> I'll admit that my secret (well, not so secret :)) agenda with this
> series is to pull more people into edk2 development from the QEMU
> community. Albeit somewhat messy, this feature isn't very large or
> complex, so I find it appropriate to "train" you with it -- if you want
> to play along, that is. :)

Laszlo, I think this is very cool. I'm interested in this kind of
"training". :)

Thanks,
-- 
Shannon

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

Re: [edk2] Could not add PCI device with big memory to aarch64 VMs

[Laszlo Ersek]

On 12/01/15 01:46, liang yan wrote:
> Hello, Laszlo,
> 
> On 11/30/2015 03:05 PM, Laszlo Ersek wrote:

[snip]

>> If you need more room (with large alignments), then there's no way
>> around supporting QEMU's 64 bit aperture, VIRT_PCIE_MMIO_HIGH (see my
>> earlier email).
> I checked the function create_pcie form pathtoqemu/hw/arm/virt.c, it has
> a flag value use_highmem(which has default "true" value).
> 
> It set base_mmio_high and size_mmio_high to device tree by function below,
> 
> qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
>  1, FDT_PCI_RANGE_IOPORT, 2, 0,
>  2, base_pio, 2, size_pio,
>  1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
>  2, base_mmio, 2, size_mmio,
>  1, FDT_PCI_RANGE_MMIO_64BIT,
>  2, base_mmio_high,
>  2, base_mmio_high, 2, size_mmio_high);
> 
> So basically, I need to add two UINT64 variables like mmio_high_base and
> mmio_high_size to PCD under function ProcessPciHost(VirtFdtDxe.c),
> and try to use this high base address and size as new aperture.
> 
> Is this correct?

It is correct, but that's only part of the story.

Parsing the 64-bit aperture from the DTB into new PCDs in
ArmVirtPkg/VirtFdtDxe is the easy part.

The hard part is modifying ArmVirtPkg/PciHostBridgeDxe, so that BAR
allocation requests (submitted by the platform independent PCI bus
driver that resides in "MdeModulePkg/Bus/Pci/PciBusDxe") are actually
serviced from this high aperture too.

> 
>> Unfortunately I can't readily help with that in the
>> "ArmVirtPkg/PciHostBridgeDxe" driver; there's no such (open-source)
>> example in the edk2 tree. Of course, I could experiment with it myself
>> -- only not right now.
> If possible, I do want to finish this part or help you finish it. I just
> work on UEFI recently, and thank you so much for your patient and detail
> explanation. I really appreciate it.
>> I guess copying and adapting the TypeMem32 logic to TypeMem64 (currently
>> short-circuited with EFI_ABORTED) could work.
> 
> Is the 32 or 64 bit determined by BAR(2-3bit) or by the PCI device
> memory size? Is there an option from QEMU?

I can't tell. :)

> Does TypeMem32 still keep  "VIRT_PCIE_MMIO" aperture and TypeMem64 use
> "VIRT_PCIE_MMIO_HIGH" aperture? or It's more like device property
> controlled from QEMU device simulation?

Good question. I don't know. I think in order to answer this question,
we should understand the whole dance between the PCI root bridge / host
bridge driver and the generic PCI bus driver.

The documentation I know of is in the Platform Init spec, version 1.4,
Volume 5, Chapter 10 "PCI Host Bridge". I've taken multiple stabs at
that chapter earlier, but I've always given up.

Sorry I can't help more, but this is new area for me as well.

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

Re: [edk2] Could not add PCI device with big memory to aarch64 VMs

[liang yan]

Hello, Laszlo,

On 11/30/2015 03:05 PM, Laszlo Ersek wrote:

On 11/30/15 19:45, liang yan wrote:


On 11/04/2015 05:53 PM, Laszlo Ersek wrote:

On 11/04/15 23:22, liang yan wrote:

Hello, Laszlo,


(2)It also has a problem that once I use a memory bigger than 256M for
ivshmem, it could not get through UEFI,
the error message is

PciBus: Discovered PCI @ [00|01|00]
 BAR[0]: Type =  Mem32; Alignment = 0xFFF;Length = 0x100;
Offset =
0x10
 BAR[1]: Type =  Mem32; Alignment = 0xFFF;Length = 0x1000; Offset
= 0x14
 BAR[2]: Type = PMem64; Alignment = 0x3FFF;Length =
0x4000;Offset = 0x18

PciBus: HostBridge->SubmitResources() - Success
ASSERT
/home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449):
((BOOLEAN)(0==1))


I am wandering if there are memory limitation for pcie devices under
Qemu environment?


Just thank you in advance and any information would be appreciated.

(CC'ing Ard.)

"Apparently", the firmware-side counterpart of QEMU commit 5125f9cd2532
has never been contributed to edk2.

Therefore the the ProcessPciHost() function in
"ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c" ignores the
DTB_PCI_HOST_RANGE_MMIO64 type range from the DTB. (Thus only
DTB_PCI_HOST_RANGE_MMIO32 is recognized as PCI MMIO aperture.)

However, even if said driver was extended to parse the new 64-bit
aperture into PCDs (which wouldn't be hard), the
ArmVirtPkg/PciHostBridgeDxe driver would still have to be taught to look
at that aperture (from the PCDs) and to serve MMIO BAR allocation
requests from it. That could be hard.

Please check edk2 commits e48f1f15b0e2^..e5ceb6c9d390, approximately,
for the background on the current code. See also chapter 13 "Protocols -
PCI Bus Support" in the UEFI spec.

Patches welcome. :)

(A separate note on ACPI vs. DT: the firmware forwards *both* from QEMU
to the runtime guest OS. However, the firmware parses only the DT for
its own purposes.)

Hello, Laszlo,

Thanks for your advices above, it's very helpful.

When debugging, I also found some problems for 32 bit PCI devices.
Hope could get some clues from you.

I checked on 512M, 1G, and 2G devices.(4G return invalid parameter
error, so I think it may be taken as a 64bit devices, is this right?).

I guess so.



First,

All devices start from base address 3EFE.

According to the below:


ProcessPciHost: Config[0x3F00+0x100) Bus[0x0..0xF]
Io[0x0+0x1)@0x3EFF Mem[0x1000+0x2EFF)@0x0

the address you mention (0x3EFE) is the *highest* inclusive
guest-phys address that an MMIO BAR can take. Not sure if that's what
you meant.


Yes, you are right, current allocation is 
EfiGcdAllocateMaxAddressSearchTopDown, so base address here is the 
highest inclusive address.



The size of the MMIO aperture for the entire PCI host is 0x2EFF
bytes: a little less than 752 MB. So devices that need 1G and 2G MMIO
BARs have no chance.


PcdPciMmio32Base is  1000=
PcdPciMmio32Size is  2EFF=


Second,

It could not get new base address when searching memory space in GCD map.

For 512M devices,

*BaseAddress = (*BaseAddress + 1 - Length) & (~AlignmentMask);

This seems to be from CoreAllocateSpace()
[MdeModulePkg/Core/Dxe/Gcd/Gcd.c]. AlignmentMask is computed from the
Alignment input parameter.

Which in turn seems to come from the BitsOfAlignment parameter computed
in NotifyPhase(), "ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c".


BaseAddress is 3EFE==

So this is the highest address (inclusive) where the 512MB BAR could end.


new BaseAddress is 1EEF==

This is the highest address (inclusive) where the 512MB BAR could start.

This should be rounded down to an 512MB alignment (I believe), and then
checked if that is still in the MMIO aperture.

512MB is 0x2000_.

Rounding 0x1EEF_ down to an integral multiple of 0x2000_ results
in zero:


~AlignmentMask is E000==
Final BaseAddress is 

And that address does not fall into the MMIO aperture.

In other words, although your size requirement of 512MB could be
theoretically satisfied from the aperture (which extends from exactly
256 MB to a little lower than 1008 MB), if you *also* require the base
address to be aligned at 512MB, then that cannot be satisfied.

Thanks for the detail explanation above, I will write email with detail 
information too. Really appreciate.



Status = CoreSearchGcdMapEntry (*BaseAddress, Length, &StartLink,
&EndLink, Map);



For bigger devices:

all stops when searching memory space because below code, Length will
bigger than MaxAddress(3EFE)

if ((Entry->BaseAddress + Length) > MaxAddress) {
  continue;
}


I also checked on ArmVirtQemu.dsc which all set to 0.

   gArmPlatformTokenSpaceGuid.PcdPciBusMin|0x0
   gArmPlatformTokenSpaceGuid.PcdPciBusMax|0x0
   gArmPlatformTokenSpaceGuid.PcdPciIoBase|0x0
   gArmPlatformTokenSpaceGuid.PcdPciIoSize|0x0
   gArmPlatformTo

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Vladimir Olovyannikov]

Thank you Andrew, Ard.

The problem was my wrong interpretation of the offset in DS-5 for the ShellPkg,
and therefore wrong entry point arithmetic.

Vladimir

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Monday, November 30, 2015 10:02 AM
> To: Vladimir Olovyannikov
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with
> step-by-step debugging in uefi
> 
> 
> > On Nov 23, 2015, at 11:31 PM, Ard Biesheuvel 
> wrote:
> >
> > On 24 November 2015 at 00:38, Vladimir Olovyannikov
> >  wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >>> Sent: Tuesday, November 10, 2015 10:04 AM
> >>> To: Vladimir Olovyannikov
> >>> Cc: Cohen, Eugene; edk2-devel@lists.01.org
> >>> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
> with
> >>> step-by-step debugging in uefi
> >>>
> >>> On 10 November 2015 at 18:41, Vladimir Olovyannikov
> >>>  wrote:
>  Ard,
>  Many thanks for your help. It works.
> 
> >>>
> >>> Great! Thanks for reporting back.
> >>>
> >>>
>  -Original Message-
>  From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>  Sent: Monday, November 09, 2015 10:31 PM
>  To: Vladimir Olovyannikov
>  Cc: Cohen, Eugene; edk2-devel@lists.01.org
>  Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
> >>> with step-by-step debugging in uefi
> 
>  On 9 November 2015 at 19:01, Vladimir Olovyannikov
>   wrote:
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Sunday, November 08, 2015 10:52 PM
> > To: Vladimir Olovyannikov
> > Cc: Cohen, Eugene; edk2-devel@lists.01.org
> > Subject: Re: [edk2] Strange behavior of the DS-5 debugger on
> AARCH64
> >>> with step-by-step debugging in uefi
> >
> > On 6 November 2015 at 21:32, Vladimir Olovyannikov
> > > wrote:
> >>> Hello Ard, Eugene,
> >>> Thank you for explanation.
> >>>
> >>> Ard, I tried the patch, but it cannot be applied to the latest 
> >>> (pulled a
> >>> minute ago, git-svn-id:
> >>> https://svn.code.sf.net/p/edk2/code/trunk/edk2@18732 6f19259b-
> 4bc3-
> >>> 4df7-8a09-765794883524)
> >>> tree: all 3 hunks failed. Which commit should I be based on to apply
> the
> >>> patch?
> >>>
> >>> Anyway I found the lines manually and changed them. However,
> when
> >>> I try to
> >>>
> >>> source /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
> >>> (0x8500,0x0028) -m (0x8000,0x4000) -a
> >>> I am getting
> >>>
> >>> ERROR(?): ValueError: need more than 1 value to unpack
> >>>  File " /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py",
> line
> >>> 94, in >
> >>>armplatform_debugger.load_all_symbols()
> >>> ERROR(CMD656):
> >>> # in
> /uefi/BroadcomPlatformPkg/NS2Pkg/Scripts/armpkg_syms.ds:2
> >>> while executing: source
> >>> /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
> >>> (0x8500,0x0028) -m (0x8000,0x4000) -a
> >>> ! The script
> /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py
> >>> failed to complete due to an error during execution of the script
> >>>
> >> [...]
> >> Ard, I got a pretty much the same issue when I tried to do some
> debugging in the ShellPkg.
> >> Except Shell I can perfectly debug everything.
> >>
> >> 1. source / uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
> (0x8500,0x0028) -m (0x8000,0x4000) -a
> >> loads symbols fine, but does not recognize any module matching the
> current PC if stopped in the shell.
> >> 2. Loading symbols with "add-symbol-file
> /uefi/Build/NS2Pkg/DEBUG_GCC49/AARCH64/ShellPkg/Application/Shell/Sh
> ell/DEBUG/Shell.dll 0xB6926000"
> >>"recognizes" modules (wrong ones though) but the source code does
> not match disassembly.
> >>
> >> So with Shell debug using DS-5 the code does not match the source.
> >> Is there a special linker setting I am missing or a technique?
> >> I am using the latest UEFI code from
> >> https://github.com/tianocore/edk2.git
> >>
> >
> > I am sorry, but since I don't have access to DS-5, I am not sure how
> > to debug this.
> >
> > Is there any way for you to figure how much the offset is between the
> > current and the correct location? I.e., by looking at the ELF asm dump
> > and the instructions around the PC? Something in the order of 0x260
> > perhaps?
> >
> 
> The map file in the build output directory is useful for tracking down these
> kind of issues.
> 
> Issues like this usually relate to the fact that PE/COFF images load the
> PE/COFF header into memory when the code is loaded, while ELF (and Mach-
> O) do not do this. This means you have to shift the link address from 0 to the
> size of the PE/COFF header (0x260 is one possible size of the PE/COFF
> header). Basically it is part of the ELF to PE/COFF

Re: [edk2] [PATCH] OvmfPkg: replace README fine print about X64 SMM S3 with PlatformPei check

[Laszlo Ersek]

On 12/01/15 00:33, Jordan Justen wrote:
> Looks good. Thanks!
> 
> Reviewed-by: Jordan Justen 

Thank you! Committed as SVN r19070.

Laszlo

> 
> On 2015-11-30 12:51:35, Laszlo Ersek wrote:
>> At the moment, the "UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module doesn't
>> support S3 resume if the platform has SMM enabled and the PEI phase is
>> built for X64. We document this in the README, but it is not conspicuous
>> enough.
>>
>> Replace the "fine print" in the README with a runtime check in
>> PlatformPei.
>>
>> Cc: Jordan Justen 
>> Suggested-by: Jordan Justen 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>>  OvmfPkg/PlatformPei/Platform.c  | 23 
>>  OvmfPkg/README  |  5 -
>>  3 files changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index ee044a2..cec19ee 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -47,6 +47,7 @@ [Guids]
>>gEfiXenInfoGuid
>>  
>>  [LibraryClasses]
>> +  BaseLib
>>DebugLib
>>HobLib
>>IoLib
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 8252dc9..657fa68 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -22,6 +22,7 @@
>>  //
>>  // The Library classes this module consumes
>>  //
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -461,6 +462,27 @@ DebugDumpCmos (
>>  }
>>  
>>  
>> +VOID
>> +S3Verification (
>> +  VOID
>> +  )
>> +{
>> +#if defined (MDE_CPU_X64)
>> +  if (FeaturePcdGet (PcdSmmSmramRequire) && mS3Supported) {
>> +DEBUG ((EFI_D_ERROR,
>> +  "%a: S3Resume2Pei doesn't support X64 PEI + SMM yet.\n", 
>> __FUNCTION__));
>> +DEBUG ((EFI_D_ERROR,
>> +  "%a: Please disable S3 on the QEMU command line (see the README),\n",
>> +  __FUNCTION__));
>> +DEBUG ((EFI_D_ERROR,
>> +  "%a: or build OVMF with \"OvmfPkgIa32X64.dsc\".\n", __FUNCTION__));
>> +ASSERT (FALSE);
>> +CpuDeadLoop ();
>> +  }
>> +#endif
>> +}
>> +
>> +
>>  /**
>>Perform Platform PEI initialization.
>>  
>> @@ -488,6 +510,7 @@ InitializePlatform (
>>  mS3Supported = TRUE;
>>}
>>  
>> +  S3Verification ();
>>BootModeInitialization ();
>>AddressWidthInitialization ();
>>  
>> diff --git a/OvmfPkg/README b/OvmfPkg/README
>> index 0f70fa7..e6137dd 100644
>> --- a/OvmfPkg/README
>> +++ b/OvmfPkg/README
>> @@ -170,11 +170,6 @@ can be used on Windows.
>>  
>>-global ICH9-LPC.disable_s3=1 \
>>  
>> -Dependent on the development status of the
>> -"UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module, S3 resume may not work in
>> -OvmfPkg/OvmfPkgX64.dsc builds. In such cases, OvmfPkg/OvmfPkgIa32X64.dsc is
>> -recommended for running X64 guests.
>> -
>>  === Network Support ===
>>  
>>  OVMF provides a UEFI network stack by default. Its lowest level driver is 
>> the
>> -- 
>> 1.8.3.1
>>

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

Re: [edk2] [PATCH] OvmfPkg: replace README fine print about X64 SMM S3 with PlatformPei check

[Jordan Justen]

Looks good. Thanks!

Reviewed-by: Jordan Justen 

On 2015-11-30 12:51:35, Laszlo Ersek wrote:
> At the moment, the "UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module doesn't
> support S3 resume if the platform has SMM enabled and the PEI phase is
> built for X64. We document this in the README, but it is not conspicuous
> enough.
> 
> Replace the "fine print" in the README with a runtime check in
> PlatformPei.
> 
> Cc: Jordan Justen 
> Suggested-by: Jordan Justen 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>  OvmfPkg/PlatformPei/Platform.c  | 23 
>  OvmfPkg/README  |  5 -
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index ee044a2..cec19ee 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -47,6 +47,7 @@ [Guids]
>gEfiXenInfoGuid
>  
>  [LibraryClasses]
> +  BaseLib
>DebugLib
>HobLib
>IoLib
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 8252dc9..657fa68 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -22,6 +22,7 @@
>  //
>  // The Library classes this module consumes
>  //
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -461,6 +462,27 @@ DebugDumpCmos (
>  }
>  
>  
> +VOID
> +S3Verification (
> +  VOID
> +  )
> +{
> +#if defined (MDE_CPU_X64)
> +  if (FeaturePcdGet (PcdSmmSmramRequire) && mS3Supported) {
> +DEBUG ((EFI_D_ERROR,
> +  "%a: S3Resume2Pei doesn't support X64 PEI + SMM yet.\n", 
> __FUNCTION__));
> +DEBUG ((EFI_D_ERROR,
> +  "%a: Please disable S3 on the QEMU command line (see the README),\n",
> +  __FUNCTION__));
> +DEBUG ((EFI_D_ERROR,
> +  "%a: or build OVMF with \"OvmfPkgIa32X64.dsc\".\n", __FUNCTION__));
> +ASSERT (FALSE);
> +CpuDeadLoop ();
> +  }
> +#endif
> +}
> +
> +
>  /**
>Perform Platform PEI initialization.
>  
> @@ -488,6 +510,7 @@ InitializePlatform (
>  mS3Supported = TRUE;
>}
>  
> +  S3Verification ();
>BootModeInitialization ();
>AddressWidthInitialization ();
>  
> diff --git a/OvmfPkg/README b/OvmfPkg/README
> index 0f70fa7..e6137dd 100644
> --- a/OvmfPkg/README
> +++ b/OvmfPkg/README
> @@ -170,11 +170,6 @@ can be used on Windows.
>  
>-global ICH9-LPC.disable_s3=1 \
>  
> -Dependent on the development status of the
> -"UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module, S3 resume may not work in
> -OvmfPkg/OvmfPkgX64.dsc builds. In such cases, OvmfPkg/OvmfPkgIa32X64.dsc is
> -recommended for running X64 guests.
> -
>  === Network Support ===
>  
>  OVMF provides a UEFI network stack by default. Its lowest level driver is the
> -- 
> 1.8.3.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch V2 0/2] UefiCpuPkg/PiSmmCpu: Enable Write Protection in SMM.

[Yao, Jiewen]

Looks good. Thank you very much for your kind help!

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, December 01, 2015 3:58 AM
To: Yao, Jiewen; edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Paolo Bonzini; Fan, Jeff
Subject: Re: [edk2] [patch V2 0/2] UefiCpuPkg/PiSmmCpu: Enable Write Protection 
in SMM.

On 11/27/15 16:23, jiewen yao wrote:
> This series patch enables write protection in SMM.
> We always set RW+P bit for page table by default, and set WP in CR0.
> So that we can use page table write-protection for code later.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Signed-off-by: "Paolo Bonzini" 
> Suggested-by: "Kinney, Michael D" 
> Tested-by: "Laszlo Ersek" 
> Reviewed-by: "Kinney, Michael D" 
> Cc: "Fan, Jeff" 
> Cc: "Kinney, Michael D" 
> Cc: "Laszlo Ersek" 
> Cc: "Paolo Bonzini" 
> 
> jiewen yao (2):
>   UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.
>   UefiCpuPkg/PiSmmCpu: Always set WP in CR0.
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S   |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c   | 14 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h  | 13 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c  | 12 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c |  8 
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm  |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c  | 14 +++---
>  11 files changed, 43 insertions(+), 30 deletions(-)
> 

I very slightly cleaned up the commit messages, and committed the series to 
SVN, revisions 19067 and 19068.

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

[edk2] Where do I find the system table address for an EFI app ?

[Shubha Ramani]

I'm trying to debug it using a debugger and the debugger is asking me this 
question.
I looked at the *.map file which is generated and I could not locate the System 
Table address.
Thanks,
Shubha Shubha D. ramanishubharam...@gmail.com
shubharam...@yahoo.com
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Laszlo Ersek]

On 11/30/15 22:59, Andrew Fish wrote:
> 
>> On Nov 30, 2015, at 12:27 PM, Laszlo Ersek  wrote:
>>
>> On 11/30/15 20:17, Alain Kalker wrote:
>>> Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and 
>>> PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:
>>>
>>> --[last lines of debug output]--
>>> DXE IPL Entry
>>>
>>> ASSERT_EFI_ERROR (Status = Invalid Parameter)
>>> ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603): !
>>> EFI_ERROR (Status)
>>> --[end]--
>>>
>>> This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb 
>>> 7.10-4.1 .
>>>
>>> To disable shadowing, I made the following change:
>>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index 44b9c79..31f73bc 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -340,6 +340,11 @@
>>>   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>> !endif
>>>
>>> +!if $(TARGET) == DEBUG
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot|FALSE
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
>>> +!endif
>>> +
>>> !ifndef $(USE_OLD_SHELL)
>>>   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
>>> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
>>> 0xB4, 0xD1 }
>>> !endif
>>
>> "Doctor, if I press here, it hurts".
>> "Well, don't press there."
>>
>> Why are you doing this in the first place?
>>
>> The documentation of "PcdShadowPeimOnBoot" goes like 
>> [MdeModulePkg/MdeModulePkg.dec]:
>>
>>>  ## Indicates if to shadow PEIM and PeiCore after memory is ready.
>>>  #  This PCD is used on other boot path except for S3 boot. 
>>>  #   TRUE  - Shadow PEIM and PeiCore after memory is ready.
>>>  #   FALSE - Not shadow PEIM after memory is ready.
>>>  # @Prompt Shadow Peim and PeiCore on boot
>>>  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|TRUE|BOOLEAN|0x30001029
>>
>> If I understand correctly, if you flip this switch to false, then the PEI 
>> core, any already loaded PEIMs, and the temporary PEI heap and stack will 
>> not be migrated to permanent PEI RAM, once permanent PEI RAM is installed. 
>> That is a completely untested path in OVMF (in fact this is the first time I 
>> ever hear of this PCD) -- why are you doing this?
>>
>> Such a change could be extremely intrusive, and (right now) I don't see any 
>> good reason to attempt to support it.
>>
>> Anyway, the assert you seem to be hitting is in the PeiLoadImageLoadImage() 
>> function:
>>
>>592   //
>>593   // If memory is installed, perform the shadow operations
>>594   //
>>595   Status = LoadAndRelocatePeCoffImage (
>>596 FileHandle,
>>597 Pe32Data,
>>598 &ImageAddress,
>>599 &ImageSize,
>>600 &ImageEntryPoint
>>601   );
>>602 
>>603   ASSERT_EFI_ERROR (Status);
>>
>> I also found a comment like this, in 
>> "MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c" (note the dependency on the 
>> PCD):
>>
>>   1143   //
>>   1144   // If memory is availble we shadow images by default 
>> for performance reasons.
>>   1145   // We call the entry point a 2nd time so the module 
>> knows it's shadowed.
>>   1146   //
>>   1147   //PERF_START (PeiServices, L"PEIM", PeimFileHandle, 0);
>>   1148   if ((Private->HobList.HandoffInformationTable->BootMode 
>> != BOOT_ON_S3_RESUME) && !PcdGetBool (PcdShadowPeimOnBoot)) {
>>   1149 //
>>   1150 // Load PEIM into Memory for Register for shadow PEIM.
>>   1151 //
>>   1152 Status = PeiLoadImage (
>>   1153PeiServices,
>>   1154PeimFileHandle,
>>   1155PEIM_STATE_REGISITER_FOR_SHADOW,
>>   1156&EntryPoint,
>>   1157&AuthenticationState
>>   1158);
>>
>> So, my take is that you might have stumbled upon a by-now untested, 
>> bit-rotted code path in edk2.
>>
>> On physical platforms the PEI phase is launched from flash, which is likely 
>> slow, so as soon as DRAM is discovered, it is -- in practice -- *always* 
>> relocated there. Which allowed the code path (enabled by the opposite PCD 
>> setting) to bit-rot.
>>
>> I think this issue is not directly related to OVMF.
>>
> 
> The strange thing about this is the failure is loading the DXE Core from the 
> DXE IPL, so all of PEI has already run and been dispatched?
> 
> #2  0x00822f0e in PeiLoadImageLoadImage at 
> /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c:603
> #3  0x00823326 in PeiLoadImageLoadImageWrapper at 
> /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c:722
> #4  0x07fc5d41 in DxeLoadCore  at 
> /home/miki/vcs/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:325
> #5  0x008218c0 in PeiCore  at 

Re: [edk2] Could not add PCI device with big memory to aarch64 VMs

[Laszlo Ersek]

On 11/30/15 19:45, liang yan wrote:
> 
> 
> On 11/04/2015 05:53 PM, Laszlo Ersek wrote:
>> On 11/04/15 23:22, liang yan wrote:
>>> Hello, Laszlo,
>>>
>>>
>>> (2)It also has a problem that once I use a memory bigger than 256M for
>>> ivshmem, it could not get through UEFI,
>>> the error message is
>>>
>>> PciBus: Discovered PCI @ [00|01|00]
>>> BAR[0]: Type =  Mem32; Alignment = 0xFFF;Length = 0x100;
>>> Offset =
>>> 0x10
>>> BAR[1]: Type =  Mem32; Alignment = 0xFFF;Length = 0x1000; Offset
>>> = 0x14
>>> BAR[2]: Type = PMem64; Alignment = 0x3FFF;Length =
>>> 0x4000;Offset = 0x18
>>>
>>> PciBus: HostBridge->SubmitResources() - Success
>>> ASSERT
>>> /home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449):
>>> ((BOOLEAN)(0==1))
>>>
>>>
>>> I am wandering if there are memory limitation for pcie devices under
>>> Qemu environment?
>>>
>>>
>>> Just thank you in advance and any information would be appreciated.
>> (CC'ing Ard.)
>>
>> "Apparently", the firmware-side counterpart of QEMU commit 5125f9cd2532
>> has never been contributed to edk2.
>>
>> Therefore the the ProcessPciHost() function in
>> "ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c" ignores the
>> DTB_PCI_HOST_RANGE_MMIO64 type range from the DTB. (Thus only
>> DTB_PCI_HOST_RANGE_MMIO32 is recognized as PCI MMIO aperture.)
>>
>> However, even if said driver was extended to parse the new 64-bit
>> aperture into PCDs (which wouldn't be hard), the
>> ArmVirtPkg/PciHostBridgeDxe driver would still have to be taught to look
>> at that aperture (from the PCDs) and to serve MMIO BAR allocation
>> requests from it. That could be hard.
>>
>> Please check edk2 commits e48f1f15b0e2^..e5ceb6c9d390, approximately,
>> for the background on the current code. See also chapter 13 "Protocols -
>> PCI Bus Support" in the UEFI spec.
>>
>> Patches welcome. :)
>>
>> (A separate note on ACPI vs. DT: the firmware forwards *both* from QEMU
>> to the runtime guest OS. However, the firmware parses only the DT for
>> its own purposes.)
> Hello, Laszlo,
> 
> Thanks for your advices above, it's very helpful.
> 
> When debugging, I also found some problems for 32 bit PCI devices.
> Hope could get some clues from you.
> 
> I checked on 512M, 1G, and 2G devices.(4G return invalid parameter
> error, so I think it may be taken as a 64bit devices, is this right?).

I guess so.

> 
> 
> First,
> 
> All devices start from base address 3EFE.

According to the below:

> ProcessPciHost: Config[0x3F00+0x100) Bus[0x0..0xF]
> Io[0x0+0x1)@0x3EFF Mem[0x1000+0x2EFF)@0x0

the address you mention (0x3EFE) is the *highest* inclusive
guest-phys address that an MMIO BAR can take. Not sure if that's what
you meant.

The size of the MMIO aperture for the entire PCI host is 0x2EFF
bytes: a little less than 752 MB. So devices that need 1G and 2G MMIO
BARs have no chance.

> PcdPciMmio32Base is  1000=
> PcdPciMmio32Size is  2EFF=
> 
> 
> Second,
> 
> It could not get new base address when searching memory space in GCD map.
> 
> For 512M devices,
> 
> *BaseAddress = (*BaseAddress + 1 - Length) & (~AlignmentMask);

This seems to be from CoreAllocateSpace()
[MdeModulePkg/Core/Dxe/Gcd/Gcd.c]. AlignmentMask is computed from the
Alignment input parameter.

Which in turn seems to come from the BitsOfAlignment parameter computed
in NotifyPhase(), "ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c".

> 
> BaseAddress is 3EFE==

So this is the highest address (inclusive) where the 512MB BAR could end.

> new BaseAddress is 1EEF==

This is the highest address (inclusive) where the 512MB BAR could start.

This should be rounded down to an 512MB alignment (I believe), and then
checked if that is still in the MMIO aperture.

512MB is 0x2000_.

Rounding 0x1EEF_ down to an integral multiple of 0x2000_ results
in zero:

> ~AlignmentMask is E000==
> Final BaseAddress is 

And that address does not fall into the MMIO aperture.

In other words, although your size requirement of 512MB could be
theoretically satisfied from the aperture (which extends from exactly
256 MB to a little lower than 1008 MB), if you *also* require the base
address to be aligned at 512MB, then that cannot be satisfied.

> 
> Status = CoreSearchGcdMapEntry (*BaseAddress, Length, &StartLink,
> &EndLink, Map);
> 
> 
> 
> For bigger devices:
> 
> all stops when searching memory space because below code, Length will
> bigger than MaxAddress(3EFE)
> 
> if ((Entry->BaseAddress + Length) > MaxAddress) {
>  continue;
> }
> 
> 
> I also checked on ArmVirtQemu.dsc which all set to 0.
> 
>   gArmPlatformTokenSpaceGuid.PcdPciBusMin|0x0
>   gArmPlatformTokenSpaceGuid.PcdPciBusMax|0x0
>   gArmPlatformTokenSpaceGuid.PcdPciIoBase|0x0
>   gArmPlatformTokenSpaceGuid.PcdPciIoSize|0x0
>   gArmPlatformTokenSpaceGuid.PcdPciIoTranslation|0x0
>   gAr

Re: [edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Andrew Fish]

> On Nov 30, 2015, at 12:27 PM, Laszlo Ersek  wrote:
> 
> On 11/30/15 20:17, Alain Kalker wrote:
>> Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and 
>> PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:
>> 
>> --[last lines of debug output]--
>> DXE IPL Entry
>> 
>> ASSERT_EFI_ERROR (Status = Invalid Parameter)
>> ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603): !
>> EFI_ERROR (Status)
>> --[end]--
>> 
>> This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb 
>> 7.10-4.1 .
>> 
>> To disable shadowing, I made the following change:
>> 
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 44b9c79..31f73bc 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -340,6 +340,11 @@
>>   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>> !endif
>> 
>> +!if $(TARGET) == DEBUG
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot|FALSE
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
>> +!endif
>> +
>> !ifndef $(USE_OLD_SHELL)
>>   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
>> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
>> 0xB4, 0xD1 }
>> !endif
> 
> "Doctor, if I press here, it hurts".
> "Well, don't press there."
> 
> Why are you doing this in the first place?
> 
> The documentation of "PcdShadowPeimOnBoot" goes like 
> [MdeModulePkg/MdeModulePkg.dec]:
> 
>>  ## Indicates if to shadow PEIM and PeiCore after memory is ready.
>>  #  This PCD is used on other boot path except for S3 boot. 
>>  #   TRUE  - Shadow PEIM and PeiCore after memory is ready.
>>  #   FALSE - Not shadow PEIM after memory is ready.
>>  # @Prompt Shadow Peim and PeiCore on boot
>>  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|TRUE|BOOLEAN|0x30001029
> 
> If I understand correctly, if you flip this switch to false, then the PEI 
> core, any already loaded PEIMs, and the temporary PEI heap and stack will not 
> be migrated to permanent PEI RAM, once permanent PEI RAM is installed. That 
> is a completely untested path in OVMF (in fact this is the first time I ever 
> hear of this PCD) -- why are you doing this?
> 
> Such a change could be extremely intrusive, and (right now) I don't see any 
> good reason to attempt to support it.
> 
> Anyway, the assert you seem to be hitting is in the PeiLoadImageLoadImage() 
> function:
> 
>592   //
>593   // If memory is installed, perform the shadow operations
>594   //
>595   Status = LoadAndRelocatePeCoffImage (
>596 FileHandle,
>597 Pe32Data,
>598 &ImageAddress,
>599 &ImageSize,
>600 &ImageEntryPoint
>601   );
>602 
>603   ASSERT_EFI_ERROR (Status);
> 
> I also found a comment like this, in 
> "MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c" (note the dependency on the 
> PCD):
> 
>   1143   //
>   1144   // If memory is availble we shadow images by default for 
> performance reasons.
>   1145   // We call the entry point a 2nd time so the module 
> knows it's shadowed.
>   1146   //
>   1147   //PERF_START (PeiServices, L"PEIM", PeimFileHandle, 0);
>   1148   if ((Private->HobList.HandoffInformationTable->BootMode 
> != BOOT_ON_S3_RESUME) && !PcdGetBool (PcdShadowPeimOnBoot)) {
>   1149 //
>   1150 // Load PEIM into Memory for Register for shadow PEIM.
>   1151 //
>   1152 Status = PeiLoadImage (
>   1153PeiServices,
>   1154PeimFileHandle,
>   1155PEIM_STATE_REGISITER_FOR_SHADOW,
>   1156&EntryPoint,
>   1157&AuthenticationState
>   1158);
> 
> So, my take is that you might have stumbled upon a by-now untested, 
> bit-rotted code path in edk2.
> 
> On physical platforms the PEI phase is launched from flash, which is likely 
> slow, so as soon as DRAM is discovered, it is -- in practice -- *always* 
> relocated there. Which allowed the code path (enabled by the opposite PCD 
> setting) to bit-rot.
> 
> I think this issue is not directly related to OVMF.
> 

The strange thing about this is the failure is loading the DXE Core from the 
DXE IPL, so all of PEI has already run and been dispatched?

#2  0x00822f0e in PeiLoadImageLoadImage at 
/home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c:603
#3  0x00823326 in PeiLoadImageLoadImageWrapper at 
/home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c:722
#4  0x07fc5d41 in DxeLoadCore  at 
/home/miki/vcs/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:325
#5  0x008218c0 in PeiCore  at 
/home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:459
#6  0x00820d43 in PeiCore  at 
/home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei

Re: [edk2] [PATCH] CorebootPayloadPkg PlatformHookLib: Fix GCC build failure

[Laszlo Ersek]

On 11/30/15 18:49, Ma, Maurice wrote:
> The patch looks good to me.   
> Reviewed-by: Maurice Ma 

Thanks. Committed to SVN as r19069, with both of our R-b tags.

Laszlo

> 
> Thanks
> Maurice
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Sunday, November 29, 2015 9:12 PM
> To: Zeng, Star; edk2-de...@ml01.01.org
> Cc: Ma, Maurice; Agyeman, Prince
> Subject: Re: [PATCH] CorebootPayloadPkg PlatformHookLib: Fix GCC build failure
> 
> On 11/30/15 02:22, Star Zeng wrote:
>> Add return status check to fix GCC build failure below.
>>
>> error: right-hand operand of comma expression has no effect 
>> [-Werror=unused-value] #define 
>> _PCD_SET_MODE_BOOL_S_PcdSerialUseMmio(Value)
>> ((_gPcd_BinaryPatch_PcdSerialUseMmio = (Value)), RETURN_SUCCESS)
>>
>> error: right-hand operand of comma expression has no effect 
>> [-Werror=unused-value] #define 
>> _PCD_SET_MODE_64_S_PcdSerialRegisterBase(Value)
>> ((_gPcd_BinaryPatch_PcdSerialRegisterBase = (Value)), RETURN_SUCCESS)
>>
>> http://article.gmane.org/gmane.comp.bios.edk2.devel/4949
>>
>> Cc: Maurice Ma 
>> Cc: Prince Agyeman 
>> Cc: Laszlo Ersek 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Suggested-by: Laszlo Ersek 
>> Signed-off-by: Star Zeng 
>> ---
>>  CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c | 12 
>> +---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git 
>> a/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c 
>> b/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
>> index 10fd332..8449997 100644
>> --- a/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
>> +++ b/CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
>> @@ -45,11 +45,17 @@ PlatformHookSerialPortInitialize (
>>}
>>  
>>if (SerialRegAccessType == 2) { //MMIO
>> -PcdSetBoolS (PcdSerialUseMmio, TRUE);
>> +Status = PcdSetBoolS (PcdSerialUseMmio, TRUE);
>>} else { //IO
>> -PcdSetBoolS (PcdSerialUseMmio, FALSE);
>> +Status = PcdSetBoolS (PcdSerialUseMmio, FALSE);  }  if 
>> + (RETURN_ERROR (Status)) {
>> +return Status;
>> +  }
>> +  Status = PcdSet64S (PcdSerialRegisterBase, (UINT64) SerialRegBase);  
>> + if (RETURN_ERROR (Status)) {
>> +return Status;
>>}
>> -  PcdSet64S (PcdSerialRegisterBase, (UINT64) SerialRegBase);
>>  
>>return RETURN_SUCCESS;
>>  }
>>
> 
> Looks good to me, although I'm not a maintainer for CorebootPayloadPkg.
> 
> Reviewed-by: Laszlo Ersek 
> 
> I've never built CorebootPayloadPkg, so I could report back about a 
> successful build only after this patch is committed, and Gerd's Jenkins 
> instance picks it up.
> 
> Thanks
> Laszlo
> 

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

[edk2] [PATCH] OvmfPkg: replace README fine print about X64 SMM S3 with PlatformPei check

[Laszlo Ersek]

At the moment, the "UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module doesn't
support S3 resume if the platform has SMM enabled and the PEI phase is
built for X64. We document this in the README, but it is not conspicuous
enough.

Replace the "fine print" in the README with a runtime check in
PlatformPei.

Cc: Jordan Justen 
Suggested-by: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
 OvmfPkg/PlatformPei/Platform.c  | 23 
 OvmfPkg/README  |  5 -
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
b/OvmfPkg/PlatformPei/PlatformPei.inf
index ee044a2..cec19ee 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -47,6 +47,7 @@ [Guids]
   gEfiXenInfoGuid
 
 [LibraryClasses]
+  BaseLib
   DebugLib
   HobLib
   IoLib
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 8252dc9..657fa68 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -22,6 +22,7 @@
 //
 // The Library classes this module consumes
 //
+#include 
 #include 
 #include 
 #include 
@@ -461,6 +462,27 @@ DebugDumpCmos (
 }
 
 
+VOID
+S3Verification (
+  VOID
+  )
+{
+#if defined (MDE_CPU_X64)
+  if (FeaturePcdGet (PcdSmmSmramRequire) && mS3Supported) {
+DEBUG ((EFI_D_ERROR,
+  "%a: S3Resume2Pei doesn't support X64 PEI + SMM yet.\n", __FUNCTION__));
+DEBUG ((EFI_D_ERROR,
+  "%a: Please disable S3 on the QEMU command line (see the README),\n",
+  __FUNCTION__));
+DEBUG ((EFI_D_ERROR,
+  "%a: or build OVMF with \"OvmfPkgIa32X64.dsc\".\n", __FUNCTION__));
+ASSERT (FALSE);
+CpuDeadLoop ();
+  }
+#endif
+}
+
+
 /**
   Perform Platform PEI initialization.
 
@@ -488,6 +510,7 @@ InitializePlatform (
 mS3Supported = TRUE;
   }
 
+  S3Verification ();
   BootModeInitialization ();
   AddressWidthInitialization ();
 
diff --git a/OvmfPkg/README b/OvmfPkg/README
index 0f70fa7..e6137dd 100644
--- a/OvmfPkg/README
+++ b/OvmfPkg/README
@@ -170,11 +170,6 @@ can be used on Windows.
 
   -global ICH9-LPC.disable_s3=1 \
 
-Dependent on the development status of the
-"UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module, S3 resume may not work in
-OvmfPkg/OvmfPkgX64.dsc builds. In such cases, OvmfPkg/OvmfPkgIa32X64.dsc is
-recommended for running X64 guests.
-
 === Network Support ===
 
 OVMF provides a UEFI network stack by default. Its lowest level driver is the
-- 
1.8.3.1

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

Re: [edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Laszlo Ersek]

On 11/30/15 20:17, Alain Kalker wrote:
> Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and 
> PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:
> 
> --[last lines of debug output]--
> DXE IPL Entry
> 
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603): !
> EFI_ERROR (Status)
> --[end]--
> 
> This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb 
> 7.10-4.1 .
> 
> To disable shadowing, I made the following change:
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 44b9c79..31f73bc 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -340,6 +340,11 @@
>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
> +!if $(TARGET) == DEBUG
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot|FALSE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
> +!endif
> +
>  !ifndef $(USE_OLD_SHELL)
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
> 0xB4, 0xD1 }
>  !endif

"Doctor, if I press here, it hurts".
"Well, don't press there."

Why are you doing this in the first place?

The documentation of "PcdShadowPeimOnBoot" goes like 
[MdeModulePkg/MdeModulePkg.dec]:

>   ## Indicates if to shadow PEIM and PeiCore after memory is ready.
>   #  This PCD is used on other boot path except for S3 boot. 
>   #   TRUE  - Shadow PEIM and PeiCore after memory is ready.
>   #   FALSE - Not shadow PEIM after memory is ready.
>   # @Prompt Shadow Peim and PeiCore on boot
>   gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|TRUE|BOOLEAN|0x30001029

If I understand correctly, if you flip this switch to false, then the PEI core, 
any already loaded PEIMs, and the temporary PEI heap and stack will not be 
migrated to permanent PEI RAM, once permanent PEI RAM is installed. That is a 
completely untested path in OVMF (in fact this is the first time I ever hear of 
this PCD) -- why are you doing this?

Such a change could be extremely intrusive, and (right now) I don't see any 
good reason to attempt to support it.

Anyway, the assert you seem to be hitting is in the PeiLoadImageLoadImage() 
function:

592   //
593   // If memory is installed, perform the shadow operations
594   //
595   Status = LoadAndRelocatePeCoffImage (
596 FileHandle,
597 Pe32Data,
598 &ImageAddress,
599 &ImageSize,
600 &ImageEntryPoint
601   );
602 
603   ASSERT_EFI_ERROR (Status);

I also found a comment like this, in 
"MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c" (note the dependency on the 
PCD):

   1143   //
   1144   // If memory is availble we shadow images by default for 
performance reasons.
   1145   // We call the entry point a 2nd time so the module knows 
it's shadowed.
   1146   //
   1147   //PERF_START (PeiServices, L"PEIM", PeimFileHandle, 0);
   1148   if ((Private->HobList.HandoffInformationTable->BootMode 
!= BOOT_ON_S3_RESUME) && !PcdGetBool (PcdShadowPeimOnBoot)) {
   1149 //
   1150 // Load PEIM into Memory for Register for shadow PEIM.
   1151 //
   1152 Status = PeiLoadImage (
   1153PeiServices,
   1154PeimFileHandle,
   1155PEIM_STATE_REGISITER_FOR_SHADOW,
   1156&EntryPoint,
   1157&AuthenticationState
   1158);

So, my take is that you might have stumbled upon a by-now untested, bit-rotted 
code path in edk2.

On physical platforms the PEI phase is launched from flash, which is likely 
slow, so as soon as DRAM is discovered, it is -- in practice -- *always* 
relocated there. Which allowed the code path (enabled by the opposite PCD 
setting) to bit-rot.

I think this issue is not directly related to OVMF.

Thanks
Laszlo

> For building the OVMF image, I used the following commands:
> 
> $ make -C BaseTools
> $ . edksetup.sh BaseTools
> $ build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -b DEBUG -t GCC49 -n 8
> 
> To get debug output from QEMU, I used the following command:
> 
> $ qemu-system-x86_64 -monitor none -serial none -chardev 
> stdio,id=biosdebug -device isa-debugcon,iobase=0x402,chardev=biosdebug -
> bios Build/OvmfX64/DEBUG_GCC49/FV/OVMF.fd
> 
> Using GDB (with a custom Python script to load symbols), I got a fill 
> backtrace.
> I will try and post the full output from QEMU boot as well as the 
> backtrace as replies to this message because many mail archives strip 
> attachments.
> 
> Kind regards,
> 
> Alain
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: [edk2] [patch V2 0/2] UefiCpuPkg/PiSmmCpu: Enable Write Protection in SMM.

[Laszlo Ersek]

On 11/27/15 16:23, jiewen yao wrote:
> This series patch enables write protection in SMM.
> We always set RW+P bit for page table by default, and set WP in CR0.
> So that we can use page table write-protection for code later.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Signed-off-by: "Paolo Bonzini" 
> Suggested-by: "Kinney, Michael D" 
> Tested-by: "Laszlo Ersek" 
> Reviewed-by: "Kinney, Michael D" 
> Cc: "Fan, Jeff" 
> Cc: "Kinney, Michael D" 
> Cc: "Laszlo Ersek" 
> Cc: "Paolo Bonzini" 
> 
> jiewen yao (2):
>   UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.
>   UefiCpuPkg/PiSmmCpu: Always set WP in CR0.
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S   |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c   | 14 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h  | 13 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c  | 12 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c |  8 
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm  |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c  | 14 +++---
>  11 files changed, 43 insertions(+), 30 deletions(-)
> 

I very slightly cleaned up the commit messages, and committed the series
to SVN, revisions 19067 and 19068.

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

Re: [edk2] [patch V2 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0.

[Kinney, Michael D]

Reviewed-by: Michael Kinney 

> -Original Message-
> From: Yao, Jiewen
> Sent: Friday, November 27, 2015 7:24 AM
> To: edk2-de...@ml01.01.org
> Cc: Yao, Jiewen ; Fan, Jeff ; 
> Kinney, Michael D ; Laszlo
> Ersek ; Paolo Bonzini 
> Subject: [patch V2 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0.
> 
> So that we can use write-protection for code later.
> 
> It is REPOST.
> It includes suggestion from "Kinney, Michael D" 
> For IA32 assembly, can we combine into a single OR
> instruction that sets both page enable and WP?
> For X64, does it make sense to use single OR instruction
> instead of 2 BTS instructions as well?
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Suggested-by: "Kinney, Michael D" 
> Reviewed-by: "Kinney, Michael D" 
> Cc: "Fan, Jeff" 
> Cc: "Kinney, Michael D" 
> Cc: "Laszlo Ersek" 
> Cc: "Paolo Bonzini" 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S   | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S| 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm  | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> index fbaa072..ec5b9a0 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> @@ -123,7 +123,7 @@ L11:
>  L12:   # as cr4.PGE is not set here, 
> refresh cr3
>  movl%eax, %cr4 # in PreModifyMtrrs() to flush 
> TLB.
>  movl%cr0, %ebx
> -orl $0x08000, %ebx # enable paging
> +orl $0x08001, %ebx # enable paging + WP
>  movl%ebx, %cr0
>  lealDSC_OFFSET(%edi),%ebx
>  movwDSC_DS(%ebx),%ax
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> index 8a12927..ac1a9b4 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> @@ -129,7 +129,7 @@ gSmiCr3 DD  ?
>  @@: ; as cr4.PGE is not set here, 
> refresh cr3
>  mov cr4, eax; in PreModifyMtrrs() to flush TLB.
>  mov ebx, cr0
> -or  ebx, 08000h ; enable paging
> +or  ebx, 08001h ; enable paging + WP
>  mov cr0, ebx
>  lea ebx, [edi + DSC_OFFSET]
>  mov ax, [ebx + DSC_DS]
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S
> index b488b74..7e9ac58 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S
> @@ -144,7 +144,7 @@ Base:
>  orb $1,%ah
>  wrmsr
>  movq%cr0, %rbx
> -btsl$31, %ebx
> +orl $0x08001, %ebx  # enable paging + WP
>  movq%rbx, %cr0
>  retf
>  LongMode:   # long mode (64-bit code) starts here
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm
> index 4f5c03c..094cf2c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm
> @@ -140,7 +140,7 @@ Base:
>  or  ah, 1
>  wrmsr
>  mov rbx, cr0
> -bts ebx, 31
> +or  ebx, 08001h; enable paging + WP
>  mov cr0, rbx
>  retf
>  @LongMode:  ; long mode (64-bit code) starts here
> --
> 1.9.5.msysgit.0

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

Re: [edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Alain Kalker]

On Mon, 30 Nov 2015 19:17:17 +, Alain Kalker wrote:

> Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and
> PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:
> 
> --[last lines of debug output]--
> DXE IPL Entry
> 
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603):
> !
> EFI_ERROR (Status)
> --[end]--
> 
> This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb
> 7.10-4.1 .

Full backtrace from GDB:

(gdb) bt full
#0  0x0082df14 in CpuDeadLoop () at /home/miki/vcs/git/edk2/
MdePkg/Library/BaseLib/CpuDeadLoop.c:37
Index = 0
#1  0x0082f55c in DebugAssert (FileName=0x834908 "/home/miki/vcs/
git/edk2/MdeModulePkg/Core/Pei/Image/Image.c", LineNumber=603, 
Description=0x8349d9 "!EFI_ERROR (Status)") at /home/miki/vcs/git/edk2/
OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c:153
Buffer = "ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/
Image/Image.c(603): !EFI_ERROR (Status)\r\n\000\000\230\000\220\000\000
\000\000\000\264\000\220\000\000\000\000\000\000.\004", '\000' , "+\037\202\000\000\000\000\000\264\000\220", '\000' , "@\000\000\000\200\000\000\000\000\373\003", '\000' , "@\002", '\000' , "\a", '\000' , "d\206\v", '\000' 
Ptr = 0x8165f6 ""
#2  0x00822f0e in PeiLoadImageLoadImage (PeiServices=0x816b90, 
FileHandle=0x900098, ImageAddressArg=0x816a98, ImageSizeArg=0x816a90, 
EntryPoint=0x816a88, AuthenticationState=0x816a74) at /home/miki/vcs/git/
edk2/MdeModulePkg/Core/Pei/Image/Image.c:603
Status = 9223372036854775810
Pe32Data = 0x9000b4
ImageAddress = 8481086
ImageSize = 0
ImageEntryPoint = 8481104
Machine = 0
SearchType1 = 18 '\022'
SearchType2 = 16 '\020'
#3  0x00823326 in PeiLoadImageLoadImageWrapper (This=0x838650, 
FileHandle=0x900098, ImageAddressArg=0x816a98, ImageSizeArg=0x816a90, 
EntryPoint=0x816a88, AuthenticationState=0x816a74) at /home/miki/vcs/git/
edk2/MdeModulePkg/Core/Pei/Image/Image.c:722
No locals.
#4  0x07fc5d41 in DxeLoadCore (This=0x7fcc350 , 
PeiServices=0x816b90, HobList={Header = 0x3fde000, HandoffInformationTable 
= 0x3fde000, MemoryAllocation = 0x3fde000, MemoryAllocationBspStore = 
0x3fde000, MemoryAllocationStack = 0x3fde000, MemoryAllocationModule = 
0x3fde000, ResourceDescriptor = 0x3fde000, Guid = 0x3fde000, 
FirmwareVolume = 0x3fde000, FirmwareVolume2 = 0x3fde000, Cpu = 0x3fde000, 
Pool = 0x3fde000, Capsule = 0x3fde000, Raw = 0x3fde000 "\001"}) at /home/
miki/vcs/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:325
Status = 0
DxeCoreFileInfo = {
  FileName = {
Data1 = 8481672, 
Data2 = 0, 
Data3 = 0, 
Data4 = "\f\000\000\000\000\000\000"
  }, 
  FileType = 0 '\000', 
  FileAttributes = 0, 
  Buffer = 0x82e8c9 , 
  BufferSize = 8481680
}
DxeCoreAddress = 134005632
DxeCoreSize = 134015024
DxeCoreEntryPoint = 0
BootMode = 0
FileHandle = 0x900098
Variable = 0x2070006
LoadFile = 0x838650
Instance = 1
AuthenticationState = 0
DataSize = 8571260
S3Resume = 0x816ab0
PeiRecovery = 0x82a72f 
MemoryData = {{
Type = 266, 
NumberOfPages = 0
  }, {
Type = 8603396, 
NumberOfPages = 0
  }, {
Type = 8481568, 
NumberOfPages = 0
  }, {
Type = 541415492, 
NumberOfPages = 541872201
  }, {
Type = 1920233029, 
NumberOfPages = 658809
  }, {
Type = 256, 
NumberOfPages = 0
  }, {
Type = 8581104, 
NumberOfPages = 0
  }, {
Type = 66972832, 
NumberOfPages = 0
  }, {
Type = 66973352, 
NumberOfPages = 0
  }, {
Type = 8619728, 
NumberOfPages = 0
  }, {
Type = 0, 
NumberOfPages = 0
  }, {
Type = 8481376, 
NumberOfPages = 0
  }, {
Type = 8577440, 
NumberOfPages = 0
  }, {
Type = 8481438, 
NumberOfPages = 0
  }, {
Type = 256, 
NumberOfPages = 0
  }, {
Type = 8481536, 
NumberOfPages = 0
  }}
#5  0x008218c0 in PeiCore (SecCoreDataPtr=0x3fddf80, PpiList=0x0, 
Data=0x3fdd788) at /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/PeiMain/
PeiMain.c:459
PrivateData = {
  Signature = 1130980688, 
  Ps = 0x816c38, 
  PpiData = {
PpiListEnd = 15, 
NotifyListEnd = 60, 
DispatchListEnd = 62, 
LastDispatchedInstall = 15, 
LastDispatchedNotify = 6

Re: [edk2] [patch V2 1/2] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.

[Kinney, Michael D]

Thanks Paolo for root causing the issue in v1  and to Laszlo for the additional 
testing!

Reviewed-by: Michael Kinney 

Mike

> -Original Message-
> From: Yao, Jiewen
> Sent: Friday, November 27, 2015 7:24 AM
> To: edk2-de...@ml01.01.org
> Cc: Yao, Jiewen ; Paolo Bonzini ; 
> Fan, Jeff ; Kinney, Michael D
> ; Laszlo Ersek 
> Subject: [patch V2 1/2] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page 
> table by default.
> 
> So that we can use write-protection for code later.
> 
> This is REPOST.
> It includes the bug fix from "Paolo Bonzini" .
> Title: fix generation of 32-bit PAE page tables
> Bits 1 and 2 are reserved in 32-bit PAE Page Directory Pointer Table
> Entries (PDPTEs); see Table 4-8 in the SDM.  With VMX extended page
> tables, the processor notices and fails the VM entry as soon as
> CR0.PG is set to 1.
> 
> And thanks "Laszlo Ersek"  to validate the fix.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Signed-off-by: "Paolo Bonzini" 
> Tested-by: "Laszlo Ersek" 
> Reviewed-by: "Kinney, Michael D" 
> Cc: "Fan, Jeff" 
> Cc: "Kinney, Michael D" 
> Cc: "Laszlo Ersek" 
> Cc: "Paolo Bonzini" 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c   | 14 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h  | 13 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c  | 12 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c |  8 
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c  | 14 +++---
>  7 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index edebaab..5d29904 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -60,7 +60,7 @@ SmmInitPageTable (
>if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
>  InitializeIDTSmmStackGuard ();
>}
> -  return Gen4GPageTable (0);
> +  return Gen4GPageTable (0, TRUE);
>  }
> 
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
> index 85756d0..767cb69 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
> @@ -24,7 +24,7 @@ InitSmmS3Cr3 (
>VOID
>)
>  {
> -  mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0);
> +  mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0, TRUE);
> 
>return ;
>  }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 06ffc6d..620b013 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -732,12 +732,14 @@ APHandler (
>Create 4G PageTable in SMRAM.
> 
>@param  ExtraPages   Additional page numbers besides for 4G 
> memory
> +  @param  Is32BitPageTable Whether the page table is 32-bit PAE
>@return PageTable Address
> 
>  **/
>  UINT32
>  Gen4GPageTable (
> -  IN  UINTN ExtraPages
> +  IN  UINTN ExtraPages,
> +  IN  BOOLEAN   Is32BitPageTable
>)
>  {
>VOID*PageTable;
> @@ -785,7 +787,7 @@ Gen4GPageTable (
>// Set Page Directory Pointers
>//
>for (Index = 0; Index < 4; Index++) {
> -Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + IA32_PG_P;
> +Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + 
> (Is32BitPageTable ? IA32_PAE_PDPTE_ATTRIBUTE_BITS :
> PAGE_ATTRIBUTE_BITS);
>}
>Pte += EFI_PAGE_SIZE / sizeof (*Pte);
> 
> @@ -793,7 +795,7 @@ Gen4GPageTable (
>// Fill in Page Directory Entries
>//
>for (Index = 0; Index < EFI_PAGE_SIZE * 4 / sizeof (*Pte); Index++) {
> -Pte[Index] = (Index << 21) + IA32_PG_PS + IA32_PG_RW + IA32_PG_P;
> +Pte[Index] = (Index << 21) | IA32_PG_PS | PAGE_ATTRIBUTE_BITS;
>}
> 
>if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
> @@ -802,7 +804,7 @@ Gen4GPageTable (
>  Pdpte = (UINT64*)PageTable;
>  for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex 
> += SIZE_2MB) {
>Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 
> 31)] & ~(EFI_PAGE_SIZE - 1));
> -  Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages + 
> IA32_PG_RW + IA32_PG_P;
> +  Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
> PAGE_ATTRIBUTE_BITS;
>//
>// Fill in Page Table Entries
>//
> @@ -819,7 +821,7 @@ Gen4GPageTable (
>  GuardPage = 0;
>}
>  } else {
> -  Pte[Index] = PageAddress + IA32_PG_RW + IA32_PG_P;
> +  Pte[Index] = PageAddress | PAGE_ATTRIBUTE_BITS;
>  }
>  PageAddress+= EFI_PAGE_SIZE;
>}
> @@ -886,7 +888,7 @@ SetCacheability (
>NewPageTabl

Re: [edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Alain Kalker]

On Mon, 30 Nov 2015 19:17:17 +, Alain Kalker wrote:

> Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and
> PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:
> 
> --[last lines of debug output]--
> DXE IPL Entry
> 
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603):
> !
> EFI_ERROR (Status)
> --[end]--
> 
> This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb
> 7.10-4.1 .

Full output from QEMU boot:

$ qemu-system-x86_64 -monitor none -serial none -chardev 
stdio,id=biosdebug -device isa-debugcon,iobase=0x402,chardev=biosdebug -
bios Build/OvmfX64/DEBUG_GCC49/FV/OVMF.fd
SecCoreStartupWithStack(0xFFFCC000, 0x818000)

Register PPI Notify: DCD0BE23-9586-40F4-B643-06522CED4EDE

Install PPI: 8C8CE578-8A3D-4F1C-9935-896185C32DD3

Install PPI: 5473C07A-3DCB-4DCA-BD6F-1E9689E7349A

The 0th FV start address is 0x082, size is 0x000E, handle is 
0x82

Register PPI Notify: 49EDB1C1-BF21-4761-BB12-EB0031AABB39

Register PPI Notify: EA7CA24B-DED5-4DAD-A389-BF827E8F9B38

Install PPI: B9E0ABFE-5979-4914-977F-6DEE78C278A6

Install PPI: DBE23AA9-A345-4B97-85B6-B226F1617389

Loading PEIM at 0x0839BC0 EntryPoint=0x0839E00 PcdPeim.efi

Install PPI: 06E81C58-4AD7-44BC-8390-F10265F72480

Install PPI: 01F34D25-4DE2-23AD-3FF3-36353FF323F1

Install PPI: 4D8B155B-C059-4C8F-8926-06FD4331DB8A

Install PPI: A60C6B59-E459-425D-9C69-0BCC9CB27D81

Loading PEIM at 0x0843640 EntryPoint=0x0843880 StatusCodePei.efi

Install PPI: 229832D3-7A30-4B36-B827-F40CB7D45436

Loading PEIM at 0x08492C0 EntryPoint=0x0849500 PlatformPei.efi

Select Item: 0x0

FW CFG Signature: 0x554D4551

Select Item: 0x1

FW CFG Revision: 0x1

QemuFwCfg interface is supported.

Platform PEIM Loaded

CMOS:

00: 05 00 50 00 15 00 02 30 11 15 26 02 00 80 00 00

10: 40 00 00 00 07 80 02 FF FF 00 00 00 00 00 00 00

20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

30: FF FF 20 00 00 07 00 20 30 00 00 00 00 12 00 00

40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Select Item: 0x19

Select Item: 0x25

S3 support was detected on QEMU

Install PPI: 7408D748-FC8C-4EE6-9288-C4BEC092A410

PublishPeiMemory: mPhysMemAddressWidth=36 PeiMemoryCap=65800 KB

PeiInstallPeiMemory MemoryBegin 0x3FBE000, MemoryLength 0x4042000

QemuInitializeRam called

Allocated Memory unaligned: Address = 0x7FD, Pages = 0x30, Type = 6 

After aligning to 0x1 bytes: Address = 0x7FD, Pages = 0x20 

Updated aligned-mem HOB with BaseAddress = 7FD, Length = 2, 
MemoryType = 6 

Created after-mem HOB with BaseAddress = 7FF, Length = 1, 
MemoryType = 7 

Reserved variable store memory: 0x7FD; size: 128kb

Platform PEI Firmware Volume Initialization

Install PPI: 49EDB1C1-BF21-4761-BB12-EB0031AABB39

Notify: PPI Guid: 49EDB1C1-BF21-4761-BB12-EB0031AABB39, Peim notify entry 
point: 82491A

The 1th FV start address is 0x090, size is 0x0090, handle is 
0x90

Select Item: 0x19

Select Item: 0x19

Temp Stack : BaseAddress=0x814000 Length=0x4000

Temp Heap  : BaseAddress=0x81 Length=0x19E0

Total temporary memory:32768 bytes.

  temporary memory stack ever used: 16384 bytes.

  temporary memory heap used:   6624 bytes.

Old Stack size 16384, New stack size 131072

Stack Hob: BaseAddress=0x3FBE000 Length=0x2

Heap Offset = 0x37CE000 Stack Offset = 0x37C6000

TemporaryRamMigration(0x81, 0x3FDA000, 0x8000)

Reinstall PPI: 8C8CE578-8A3D-4F1C-9935-896185C32DD3

Reinstall PPI: 5473C07A-3DCB-4DCA-BD6F-1E9689E7349A

Reinstall PPI: B9E0ABFE-5979-4914-977F-6DEE78C278A6

Install PPI: F894643D-C449-42D1-8EA8-85BDD8C65BDE

Loading PEIM at 0x0856F00 EntryPoint=0x0857140 DxeIpl.efi

Loading PEIM at 0x7FC5000 EntryPoint=0x7FC5240 DxeIpl.efi

Install PPI: 0AE8CE5D-E448-4437-A8D7-EBF5F194F731

Install PPI: 1A36E4E7-FAB6-476A-8E75-695A0576FDD7

Loading PEIM at 0x0861140 EntryPoint=0x0861380 S3Resume2Pei.efi

Install PPI: 6D582DBC-DB85-4514-8FCC-5ADF6227B147

DXE IPL Entry



ASSERT_EFI_ERROR (Status = Invalid Parameter)

ASSERT /home//vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603): !
EFI_ERROR (Status)

qemu: terminating on signal 2

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

[edk2] OvmfPkg: ASSERT hit when booting in QEMU with PcdShadowPeimOnS3Boot and PcdShadowPeimOnBoot set to FALSE

[Alain Kalker]

Booting a debug build of OVMF on QEMU, with PcdShadowPeimOnS3Boot and 
PcdShadowPeimOnBoot set to FALSE, i'm hitting the following assertion:

--[last lines of debug output]--
DXE IPL Entry

ASSERT_EFI_ERROR (Status = Invalid Parameter)
ASSERT /home/miki/vcs/git/edk2/MdeModulePkg/Core/Pei/Image/Image.c(603): !
EFI_ERROR (Status)
--[end]--

This is on Arch Linux (x86_64), package versions: qemu 2.4.1-1, gdb 
7.10-4.1 .

To disable shadowing, I made the following change:

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 44b9c79..31f73bc 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -340,6 +340,11 @@
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
 
+!if $(TARGET) == DEBUG
+  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot|FALSE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
+!endif
+
 !ifndef $(USE_OLD_SHELL)
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
0xB4, 0xD1 }
 !endif

For building the OVMF image, I used the following commands:

$ make -C BaseTools
$ . edksetup.sh BaseTools
$ build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -b DEBUG -t GCC49 -n 8

To get debug output from QEMU, I used the following command:

$ qemu-system-x86_64 -monitor none -serial none -chardev 
stdio,id=biosdebug -device isa-debugcon,iobase=0x402,chardev=biosdebug -
bios Build/OvmfX64/DEBUG_GCC49/FV/OVMF.fd

Using GDB (with a custom Python script to load symbols), I got a fill 
backtrace.
I will try and post the full output from QEMU boot as well as the 
backtrace as replies to this message because many mail archives strip 
attachments.

Kind regards,

Alain

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

Re: [edk2] [patch V2 0/2] UefiCpuPkg/PiSmmCpu: Enable Write Protection in SMM.

[Laszlo Ersek]

On 11/27/15 16:23, jiewen yao wrote:
> This series patch enables write protection in SMM.
> We always set RW+P bit for page table by default, and set WP in CR0.
> So that we can use page table write-protection for code later.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Signed-off-by: "Paolo Bonzini" 
> Suggested-by: "Kinney, Michael D" 
> Tested-by: "Laszlo Ersek" 
> Reviewed-by: "Kinney, Michael D" 
> Cc: "Fan, Jeff" 
> Cc: "Kinney, Michael D" 
> Cc: "Laszlo Ersek" 
> Cc: "Paolo Bonzini" 
> 
> jiewen yao (2):
>   UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.
>   UefiCpuPkg/PiSmmCpu: Always set WP in CR0.
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S   |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c   | 14 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h  | 13 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c  | 12 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c |  8 
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S|  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm  |  2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c  | 14 +++---
>  11 files changed, 43 insertions(+), 30 deletions(-)
> 

Now that the OVMF SMM (v5) series has been committed, I retested this
series, with both the Ia32 and the Ia32X64 build of OVMF (with -D
SMM_REQUIRE of course). I'd like to confirm that it works.

Series
Tested-by: Laszlo Ersek 

If Mike is okay with this version, I'm ready to commit it to SVN.

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

Re: [edk2] [PATCH v5 00/33] OvmfPkg: support SMM for better security

[Laszlo Ersek]

On 11/27/15 03:34, Laszlo Ersek wrote:
> Public branch: .
> 
> The big change in this version is that Mike has flattened, cleaned up,
> and incorporated CpuS3DataDxe from OvmfPkg/QuarkPort to UefiCpuPkg.
> (Huge kudos for that again!) The OvmfPkg/QuarkPort directory is
> therefore gone, as are any other mentions of Quark in the source code
> and in the commit messages.
> 
> Another important change is that the series no longer introduces a
> separate null implementation of EFI_PEI_SMM_COMMUNICATION_PPI under
> OvmfPkg -- kudos to Jiewen for suggesting and accepting a PEI LockBoxLib
> patch that has made this possible.
> 
> In addition, the series addresses all agreed-upon v4 feedback, and picks
> up all review tags too (some of which were given dependent on addressing
> said review feedback).
> 
> In total, the following patches have seen modifications -- please see
> their Notes sections individually for the changes:
> 
>   [PATCH v5 02/33] OvmfPkg: Sec: force reinit of BaseExtractGuidedSectionLib 
> handler table
>   [PATCH v5 07/33] OvmfPkg: add PEIM for providing TSEG-as-SMRAM during PEI
>   [PATCH v5 08/33] OvmfPkg: add DXE_DRIVER for providing TSEG-as-SMRAM during 
> boot-time DXE
>   [PATCH v5 09/33] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a 
> DXE_RUNTIME_DRIVER
>   [PATCH v5 10/33] OvmfPkg: pull in the SMM IPL and SMM core
>   [PATCH v5 13/33] OvmfPkg: LockBoxLib: -D SMM_REQUIRE excludes our fake 
> lockbox
>   [PATCH v5 14/33] OvmfPkg: PlatformPei: don't allocate fake lockbox if 
> SMM_REQUIRE
>   [PATCH v5 19/33] OvmfPkg: select LocalApicLib instance with x2apic support
>   [PATCH v5 21/33] OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg
>   [PATCH v5 22/33] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits
>   [PATCH v5 27/33] OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE
>   [PATCH v5 28/33] OvmfPkg: build PiSmmCpuDxeSmm for -D SMM_REQUIRE
>   [PATCH v5 33/33] OvmfPkg: README: document SMM status
> 
> In the process I had to drop two R-b tags from Mike, because I had to
> refresh Paolo's patches (marked #21 and #22 above) against the meanwhile
> extended "UefiCpuPkg/Library/SmmCpuFeaturesLib" instance. Although the
> end result in the "OvmfPkg/Library/SmmCpuFeaturesLib" instance is
> exactly the same as before -- modulo the whitespace and comment
> cleanups, and the two new empty APIs now inherited from UefiCpuPkg --, I
> felt the review process would be cleaner and more integral if Mike took
> a look again. Much appreciated.
> 
> Accounting for all of the above, only the following patches need
> reviews:
> 
>   [PATCH v5 21/33] OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg
>   [PATCH v5 22/33] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits
>   [PATCH v5 27/33] OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE
> 
> After which I plan to commit the series.
> 
> Cc: Paolo Bonzini 
> Cc: Jordan Justen 
> Cc: Michael Kinney 
> Cc: Jiewen Yao 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (27):
>   OvmfPkg: introduce -D SMM_REQUIRE and PcdSmmSmramRequire
>   OvmfPkg: Sec: force reinit of BaseExtractGuidedSectionLib handler
> table
>   OvmfPkg: Sec: assert the build-time calculated end of the scratch
> buffer
>   OvmfPkg: decompress FVs on S3 resume if SMM_REQUIRE is set
>   OvmfPkg: PlatformPei: allow caching in AddReservedMemoryBaseSizeHob()
>   OvmfPkg: PlatformPei: account for TSEG size with PcdSmmSmramRequire
> set
>   OvmfPkg: add PEIM for providing TSEG-as-SMRAM during PEI
>   OvmfPkg: add DXE_DRIVER for providing TSEG-as-SMRAM during boot-time
> DXE
>   OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER
>   OvmfPkg: pull in the SMM IPL and SMM core
>   OvmfPkg: pull in CpuIo2Smm driver
>   OvmfPkg: AcpiS3SaveDxe: don't fake LockBox protocol if SMM_REQUIRE
>   OvmfPkg: LockBoxLib: -D SMM_REQUIRE excludes our fake lockbox
>   OvmfPkg: PlatformPei: don't allocate fake lockbox if SMM_REQUIRE
>   OvmfPkg: LockBox: use SMM stack with -D SMM_REQUIRE
>   OvmfPkg: resolve ReportStatusCodeLib for DXE_SMM_DRIVER modules
>   OvmfPkg: resolve CpuExceptionHandlerLib for DXE_SMM_DRIVER modules
>   OvmfPkg: select LocalApicLib instance with x2apic support
>   OvmfPkg: set gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection to
> FALSE
>   OvmfPkg: any AP in SMM should not wait for the BSP for more than 100
> ms
>   OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE
>   OvmfPkg: build PiSmmCpuDxeSmm for -D SMM_REQUIRE
>   OvmfPkg: QemuFlashFvbServicesRuntimeDxe: add DXE_SMM_DRIVER build
>   OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D SMM_REQUIRE
>   OvmfPkg: consolidate variable driver stack in DSC and FDF files
>   OvmfPkg: pull in SMM-based variable driver stack
>   OvmfPkg: README: document SMM status
> 
> Michael Kinney (1):
>   OvmfPkg: resolve DebugAgentLib for DXE_SMM_DRIVER modules
> 
> Paolo Bonzini (5):
>   OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg
>   OvmfPkg: SmmCpuFeaturesLib: 

Re: [edk2] Could not add PCI device with big memory to aarch64 VMs

[liang yan]

On 11/04/2015 05:53 PM, Laszlo Ersek wrote:

On 11/04/15 23:22, liang yan wrote:

Hello, Laszlo,


(2)It also has a problem that once I use a memory bigger than 256M for
ivshmem, it could not get through UEFI,
the error message is

PciBus: Discovered PCI @ [00|01|00]
BAR[0]: Type =  Mem32; Alignment = 0xFFF;Length = 0x100; Offset =
0x10
BAR[1]: Type =  Mem32; Alignment = 0xFFF;Length = 0x1000; Offset
= 0x14
BAR[2]: Type = PMem64; Alignment = 0x3FFF;Length =
0x4000;Offset = 0x18

PciBus: HostBridge->SubmitResources() - Success
ASSERT
/home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449): 
((BOOLEAN)(0==1))


I am wandering if there are memory limitation for pcie devices under
Qemu environment?


Just thank you in advance and any information would be appreciated.

(CC'ing Ard.)

"Apparently", the firmware-side counterpart of QEMU commit 5125f9cd2532
has never been contributed to edk2.

Therefore the the ProcessPciHost() function in
"ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c" ignores the
DTB_PCI_HOST_RANGE_MMIO64 type range from the DTB. (Thus only
DTB_PCI_HOST_RANGE_MMIO32 is recognized as PCI MMIO aperture.)

However, even if said driver was extended to parse the new 64-bit
aperture into PCDs (which wouldn't be hard), the
ArmVirtPkg/PciHostBridgeDxe driver would still have to be taught to look
at that aperture (from the PCDs) and to serve MMIO BAR allocation
requests from it. That could be hard.

Please check edk2 commits e48f1f15b0e2^..e5ceb6c9d390, approximately,
for the background on the current code. See also chapter 13 "Protocols -
PCI Bus Support" in the UEFI spec.

Patches welcome. :)

(A separate note on ACPI vs. DT: the firmware forwards *both* from QEMU
to the runtime guest OS. However, the firmware parses only the DT for
its own purposes.)

Hello, Laszlo,

Thanks for your advices above, it's very helpful.

When debugging, I also found some problems for 32 bit PCI devices.
Hope could get some clues from you.

I checked on 512M, 1G, and 2G devices.(4G return invalid parameter 
error, so I think it may be taken as a 64bit devices, is this right?).



First,

All devices start from base address 3EFE.

ProcessPciHost: Config[0x3F00+0x100) Bus[0x0..0xF] 
Io[0x0+0x1)@0x3EFF Mem[0x1000+0x2EFF)@0x0


PcdPciMmio32Base is  1000=
PcdPciMmio32Size is  2EFF=


Second,

It could not get new base address when searching memory space in GCD map.

For 512M devices,

*BaseAddress = (*BaseAddress + 1 - Length) & (~AlignmentMask);

BaseAddress is 3EFE==
new BaseAddress is 1EEF==
~AlignmentMask is E000==
Final BaseAddress is 

Status = CoreSearchGcdMapEntry (*BaseAddress, Length, &StartLink, 
&EndLink, Map);




For bigger devices:

all stops when searching memory space because below code, Length will 
bigger than MaxAddress(3EFE)


if ((Entry->BaseAddress + Length) > MaxAddress) {
 continue;
}


I also checked on ArmVirtQemu.dsc which all set to 0.

  gArmPlatformTokenSpaceGuid.PcdPciBusMin|0x0
  gArmPlatformTokenSpaceGuid.PcdPciBusMax|0x0
  gArmPlatformTokenSpaceGuid.PcdPciIoBase|0x0
  gArmPlatformTokenSpaceGuid.PcdPciIoSize|0x0
  gArmPlatformTokenSpaceGuid.PcdPciIoTranslation|0x0
  gArmPlatformTokenSpaceGuid.PcdPciMmio32Base|0x0
  gArmPlatformTokenSpaceGuid.PcdPciMmio32Size|0x0
  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x0


Do you think I should change from PcdPciMmio32Base and PcdPciMmio32Size, 
or do some change for GCD entry list, so it could allocate resources for 
PCI devices(CoreSearchGcdMapEntry)?



Looking forward to your reply.


Thanks,
Liang


Thanks
Laszlo



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

Re: [edk2] [PATCH v2 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

Shannon,

On 11/30/15 13:37, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Here we add the memory space for the memory nodes except the lowest one
> in FDT. So these spaces will show up in the UEFI memory map.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Shannon Zhao 
> ---
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 72 
> 
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> index 74f80d1..11a9b5a 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -274,6 +275,73 @@ ProcessPciHost (
>return EFI_SUCCESS;
>  }
>  
> +/**
> +  Process the device tree node whose "device_type" property is "memory" and 
> add
> +the memory range of this node to System RAM space.
> +
> +  param[in] DeviceTreeBase  Pointer to the device tree.
> +
> +  param[in] NodeOffset of the device tree node.
> +
> +  @retval TRUE  The "device_type" property of this device tree 
> node
> +  is "memory" either adding successfully or
> +  unsuccessfully.
> +
> +  @retval FALSE The "device_type" property of this device tree 
> node
> +  is not "memory"
> +**/
> +STATIC
> +BOOLEAN
> +EFIAPI
> +AddMemorySpaceToSystemRam (
> +  IN CONST VOID *DeviceTreeBase,
> +  IN INT32  Node
> +  )
> +{
> +  CONST CHAR8  *Type;
> +  INT32Len;
> +  CONST VOID   *RegProp;
> +  EFI_STATUS   Status;
> +  UINT64   CurBase;
> +  UINT64   CurSize;
> +
> +  //
> +  // Check for memory node and add the memory spaces expect the lowest one
> +  //
> +  Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +  if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +//
> +// Get the 'reg' property of this node. For now, we will assume
> +// two 8 byte quantities for base and size, respectively.
> +//
> +RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +  CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +  CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
> +
> +  if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
> +Status = gDS->AddMemorySpace (
> +EfiGcdMemoryTypeSystemMemory,
> +CurBase, CurSize,
> +EFI_MEMORY_WB | EFI_MEMORY_WC |
> +EFI_MEMORY_WT | EFI_MEMORY_UC);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((EFI_D_ERROR, "%a: Failed to add System RAM @ 0x%lx - 
> 0x%lx\n",
> +__FUNCTION__, CurBase, CurBase + CurSize - 1));
> +} else {
> +  DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
> +__FUNCTION__, CurBase, CurBase + CurSize - 1));
> +}
> +  }
> +}
> +
> +return TRUE;
> +  }
> +
> +  return FALSE;
> +}
>  
>  EFI_STATUS
>  EFIAPI
> @@ -332,6 +400,10 @@ InitializeVirtFdtDxe (
>break;
>  }
>  
> +if (AddMemorySpaceToSystemRam (DeviceTreeBase, Node)) {
> +  continue;
> +}
> +
>  Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
>  if (Type == NULL) {
>continue;
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> index ee2503a..0e6927b 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -43,12 +43,16 @@
>VirtioMmioDeviceLib
>HobLib
>XenIoMmioLib
> +  DxeServicesTableLib
>  
>  [Guids]
>gFdtTableGuid
>gVirtioMmioTransportGuid
>gFdtHobGuid
>  
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +
>  [Pcd]
>gArmVirtTokenSpaceGuid.PcdArmPsciMethod
>gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
> 

Thank you -- this looks better. Unfortunately the
SetMemorySpaceAttributes() question still has to be addressed, and we're
still discussing with Ard which way to go about that. Once we have an
agreement, I'll spell out the further requirements for this.

The "do it all in PEI with HOBs" idea is out, now we're trying to decide
if (a) we should do it in VirtFdtDxe, and update the APRIORI DXE block
in the FDF file so that CpuDxe gets dispatched before VirtFdtDxe, or (b)
if we should move this code to a separate DXE driver that depends on the
CPU architectural protocol installed by CpuDxe. Whatever the agreement,
this patch will need to be further updated.

I ask for your patience with this -- it's messier than I would like. I
hope your NUMA development can proceed even until we converge with the
help of these WIP patches.

I'll admit that my secret (well, not so secret :)) agenda wit

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Andrew Fish]

> On Nov 23, 2015, at 11:31 PM, Ard Biesheuvel  
> wrote:
> 
> On 24 November 2015 at 00:38, Vladimir Olovyannikov
>  wrote:
>> 
>> 
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Tuesday, November 10, 2015 10:04 AM
>>> To: Vladimir Olovyannikov
>>> Cc: Cohen, Eugene; edk2-devel@lists.01.org
>>> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with
>>> step-by-step debugging in uefi
>>> 
>>> On 10 November 2015 at 18:41, Vladimir Olovyannikov
>>>  wrote:
 Ard,
 Many thanks for your help. It works.
 
>>> 
>>> Great! Thanks for reporting back.
>>> 
>>> 
 -Original Message-
 From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
 Sent: Monday, November 09, 2015 10:31 PM
 To: Vladimir Olovyannikov
 Cc: Cohen, Eugene; edk2-devel@lists.01.org
 Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
>>> with step-by-step debugging in uefi
 
 On 9 November 2015 at 19:01, Vladimir Olovyannikov
  wrote:
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Sunday, November 08, 2015 10:52 PM
> To: Vladimir Olovyannikov
> Cc: Cohen, Eugene; edk2-devel@lists.01.org
> Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64
>>> with step-by-step debugging in uefi
> 
> On 6 November 2015 at 21:32, Vladimir Olovyannikov
> > wrote:
>>> Hello Ard, Eugene,
>>> Thank you for explanation.
>>> 
>>> Ard, I tried the patch, but it cannot be applied to the latest (pulled a
>>> minute ago, git-svn-id:
>>> https://svn.code.sf.net/p/edk2/code/trunk/edk2@18732 6f19259b-4bc3-
>>> 4df7-8a09-765794883524)
>>> tree: all 3 hunks failed. Which commit should I be based on to apply the
>>> patch?
>>> 
>>> Anyway I found the lines manually and changed them. However, when
>>> I try to
>>> 
>>> source /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
>>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>>> I am getting
>>> 
>>> ERROR(?): ValueError: need more than 1 value to unpack
>>>  File " /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py", line
>>> 94, in >
>>>armplatform_debugger.load_all_symbols()
>>> ERROR(CMD656):
>>> # in /uefi/BroadcomPlatformPkg/NS2Pkg/Scripts/armpkg_syms.ds:2
>>> while executing: source
>>> /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f
>>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>>> ! The script /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py
>>> failed to complete due to an error during execution of the script
>>> 
>> [...]
>> Ard, I got a pretty much the same issue when I tried to do some debugging in 
>> the ShellPkg.
>> Except Shell I can perfectly debug everything.
>> 
>> 1. source / uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f 
>> (0x8500,0x0028) -m (0x8000,0x4000) -a
>> loads symbols fine, but does not recognize any module matching the 
>> current PC if stopped in the shell.
>> 2. Loading symbols with "add-symbol-file 
>> /uefi/Build/NS2Pkg/DEBUG_GCC49/AARCH64/ShellPkg/Application/Shell/Shell/DEBUG/Shell.dll
>>  0xB6926000"
>>"recognizes" modules (wrong ones though) but the source code does not 
>> match disassembly.
>> 
>> So with Shell debug using DS-5 the code does not match the source.
>> Is there a special linker setting I am missing or a technique?
>> I am using the latest UEFI code from
>> https://github.com/tianocore/edk2.git
>> 
> 
> I am sorry, but since I don't have access to DS-5, I am not sure how
> to debug this.
> 
> Is there any way for you to figure how much the offset is between the
> current and the correct location? I.e., by looking at the ELF asm dump
> and the instructions around the PC? Something in the order of 0x260
> perhaps?
> 

The map file in the build output directory is useful for tracking down these 
kind of issues. 

Issues like this usually relate to the fact that PE/COFF images load the 
PE/COFF header into memory when the code is loaded, while ELF (and Mach-O) do 
not do this. This means you have to shift the link address from 0 to the size 
of the PE/COFF header (0x260 is one possible size of the PE/COFF header). 
Basically it is part of the ELF to PE/COFF conversion magic. Maybe the ELF to 
PE/COFF conversion tool and the debugger script our out of sync? 

The other thing to look out for is TE (Tiano Executable) images. They have a 
stripped down version of a PE/COFF header to save space, and they are mostly 
used for PEI modules that run from FLASH. You have to add a negative adjustment 
to get the symbols loaded correctly. 

Thanks,

Andrew Fish

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

___
edk2-devel mailing list
edk2-deve

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 12:48, Ard Biesheuvel wrote:
> On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
>> On 11/30/15 11:03, Ard Biesheuvel wrote:
> [...]
>>>
>>> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
>>> HOBs the first time we enumerate the nodes?
>>
>> I didn't suggest it for two reasons.
>>
>> (1) I recalled that last time we had had a very long discussion about how 
>> the DXE core selects the initial range for memory allocations (for its own 
>> purposes, primarily). I had trouble remembering all the details now. So 
>> there were three options for me:
>>
>> - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE 
>> core picks will be good for us
>>
>> - recommend the HOBs, and actually review what the DXE core does (it was 
>> modified a little bit last time)
>>
>> - recommend a single HOB (which implies the DXE core gets initialized 
>> exactly the same as before, no thinking or code browsing needed), and delay 
>> the installation of the addtional ranges until later.
>>
>> I picked the last option for simplicity.
>>
>> (2) The other reason is that I don't think that the HOB approach would solve 
>> the question of caching attributes. I don't think the DXE core tries to set 
>> any attributes on its own when the GCD is primed from the HOBs.
>>
>> Remember we have
>>
>> InitializeMemory()  
>> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>   ArmPlatformInitializeSystemMemory()   
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>> PcdSet64 (PcdSystemMemorySize, ...)
>>   MemoryPeim()  
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>> PcdGet64 (PcdSystemMemorySize)
>> BuildResourceDescriptorHob()
>> InitMmu()   
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>   ArmPlatformGetVirtualMemoryMap()  
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>> PcdGet64 (PcdSystemMemorySize)
>>   ArmConfigureMmu() 
>> [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
>>
>> In other words, all of the following happens in the memory init PEIM (which 
>> comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg 
>> library instances):
>> - the in-DRAM DTB is initially parsed for the memory node
>> - the size PCD is set
>> - the base PCD is verified
>> - a memory descriptor HOB is built, dependent on the PCDs
>> - a virtual memory map is built, with caching attributes, dependent on the 
>> PCDs
>> - the MMU is configured
>>
>> Therefore we have a nice fast caching setting for the "base" memory node 
>> only because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() -- 
>> dependent on the PCDs --, not because the DXE core covers it when it 
>> processes the HOBs. So if you want to process several memory nodes *for 
>> real* in the the memory init PEIM, then not only do you have to create the 
>> HOBs there, but also extend the virtual memory map for the MMU similarly. 
>> And a fixed count of PCDs won't be enough to carry the information from the 
>> DTB to ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist 
>> that connects the DTB with the virtual memory map.
>>
>> This is what I meant in my original response by "having more flexibility in 
>> DXE".
>>
> 
> Yes, that does sound like a lot of work for little gain. But I am not
> too crazy about adding more 'A PRIORI' modules, to avoid ending up in
> dependency hell later.

I agree.

Unfortunately, the only other possibility I can see is a separate driver
that has a DepEx on the CPU architectural protocol, iterates over the
FDT again, and installs the memory ranges (including setting the caching).

(I also thought about delaying this logic within VirtFdtDxe: set up a
protocol notify callback on the CPU arch protocol, and do the same thing
in the callback. Unfortunately, this is exactly what the DXE core does
too, and the ordering between protocol notify callbacks is not
specified. So if ours ran first, there would be trouble.)

Take your pick. If you opt for the separate driver / DepEx approach, I
can certainly agree with that as well. It would take a bit more
boilerplate (separate directory, separate INF file), but it would be
very clean, as far as dispatch order and driver responsibility go.

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

Re: [edk2] [PATCH v5 27/33] OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE

[Kinney, Michael D]

Reviewed-by: Michael Kinney 

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, November 26, 2015 6:35 PM
> To: edk2-de...@ml01.01.org
> Cc: Justen, Jordan L ; Kinney, Michael D 
> 
> Subject: [PATCH v5 27/33] OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE
> 
> The PiSmmCpuDxeSmm driver from UefiCpuPkg depends on the ACPI_CPU_DATA
> structure -- created by a platform- and CPU-specific driver -- in order to
> support ACPI S3. The address of this structure is communicated through the
> dynamic PCD PcdCpuS3DataAddress.
> 
> The "UefiCpuPkg/Include/AcpiCpuData.h" header file documents the fields of
> this structure in detail.
> 
> The simple/generic "UefiCpuPkg/CpuS3DataDxe" driver creates and populates
> the structure in a conformant way, and it co-operates well with
> PiSmmCpuDxeSmm, for OVMF's purposes.
> 
>  PlatformBdsLib  CpuS3DataDxe PiSmmCpuDxeSmmS3ResumePei
>  (DXE_DRIVER)(DXE_DRIVER) (DXE_SMM_DRIVER)  (PEIM)
>  --  ---    --
> normal   collects data
> boot except MTRR
>  settings into
>  ACPI_CPU_DATA
> 
>  sets
>  PcdCpuS3Da...
> 
>  signals
>  End-of-Dxe
> |
> +--> collects MTRR
>  settings into
>  ACPI_CPU_DATA
> 
>  installs
>  [Dxe]Smm
>  ReadyToLock
> |
> +---> fetches
>   PcdCpuS3Dat...
> 
>   copies
>   ACPI_CPU_DATA
>   into SMRAM
> 
> runtime
> 
> S3
> suspend
> 
> S3  transfers
> resume  control to
> PiSmmCpuDxe...
> |
>   uses <+
>   ACPI_CPU_DATA
>   from SMRAM
> 
> Cc: Jordan Justen 
> Cc: Michael Kinney 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> v5:
> - rename patch from
>   "OvmfPkg: build QuarkPort/CpuS3DataDxe for -D SMM_REQUIRE"
> - get driver from UefiCpuPkg, rather than OvmfPkg/QuarkPort
> - incorporate data / control flow information in the commit message
>   from:
>   "OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe"
>   "OvmfPkg: QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.MtrrTable"
> 
> v3:
> - new in v3; split out from
>   "OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe".
> 
>  OvmfPkg/OvmfPkgIa32.dsc| 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>  OvmfPkg/OvmfPkgX64.dsc | 1 +
>  OvmfPkg/OvmfPkgIa32.fdf| 1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
>  OvmfPkg/OvmfPkgX64.fdf | 1 +
>  6 files changed, 6 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 485966a..5a63e5b 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -716,6 +716,7 @@ [Components]
>  !if $(SMM_REQUIRE) == TRUE
>OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> 
>#
># SMM Initial Program Load (a DXE_RUNTIME_DRIVER)
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index bd76303..b99a1c0 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -723,6 +723,7 @@ [Components.X64]
>  !if $(SMM_REQUIRE) == TRUE
>OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> 
>#
># SMM Initial Program Load (a DXE_RUNTIME_DRIVER)
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 653f2a1..cd52bda 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -721,6 +721,7 @@ [Components]
>  !if $(SMM_REQUIRE) == TRUE
>OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> 
>#
># SMM Initial Program Load (a DXE_RUNTIME_DRIVER)
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 53ddae3..83702a1 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -358,6 +358,7 @@ [FV.DXEFV]
>  !if $(SMM_REQUIRE) == TRUE
>  INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>  INF  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +INF  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
>  INF  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
>  INF  MdeModulePkg/Core/P

Re: [edk2] [PATCH v5 22/33] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits

[Kinney, Michael D]

Reviewed-by: Michael Kinney 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, November 26, 2015 6:35 PM
> To: edk2-de...@ml01.01.org
> Cc: Paolo Bonzini 
> Subject: [edk2] [PATCH v5 22/33] OvmfPkg: SmmCpuFeaturesLib: remove 
> unnecessary bits
> 
> From: Paolo Bonzini 
> 
> SMRR, MTRR, and SMM Feature Control support is not needed on a virtual
> platform.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 
> Acked-by: Laszlo Ersek 
> [ler...@redhat.com: insert space between ASSERT and (), convert to CRLF,
>  refresh against SVN r18958]
> Cc: Paolo Bonzini 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> v5:
> - refresh patch against SVN r18958 ("Add 2 APIs in SmmCpuFeaturesLib")
> - drop Mike's R-b consequently
> 
> v3:
> - new in v3
> 
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |   4 -
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 233 
> ++--
>  2 files changed, 17 insertions(+), 220 deletions(-)
> 
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index deb08bf..aa4792c 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -32,8 +32,4 @@ [Packages]
>  [LibraryClasses]
>BaseLib
>PcdLib
> -  MemoryAllocationLib
>DebugLib
> -
> -[Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber## 
> SOMETIMES_CONSUMES
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 3e480e1..3b6f186 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -15,58 +15,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
> 
> -//
> -// Machine Specific Registers (MSRs)
> -//
> -#define  SMM_FEATURES_LIB_IA32_MTRR_CAP0x0FE
> -#define  SMM_FEATURES_LIB_IA32_FEATURE_CONTROL 0x03A
> -#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE   0x1F2
> -#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK   0x1F3
> -#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE  0x0A0
> -#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK  0x0A1
> -#defineEFI_MSR_SMRR_MASK   0xF000
> -#defineEFI_MSR_SMRR_PHYS_MASK_VALIDBIT11
> -#define  SMM_FEATURES_LIB_SMM_FEATURE_CONTROL  0x4E0
> -
> -//
> -// MSRs required for configuration of SMM Code Access Check
> -//
> -#define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
> -#define   SMM_CODE_ACCESS_CHK_BIT  BIT58
> -
> -//
> -// Set default value to assume SMRR is not supported
> -//
> -BOOLEAN  mSmrrSupported = FALSE;
> -
> -//
> -// Set default value to assume MSR_SMM_FEATURE_CONTROL is not supported
> -//
> -BOOLEAN  mSmmFeatureControlSupported = FALSE;
> -
> -//
> -// Set default value to assume IA-32 Architectural MSRs are used
> -//
> -UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> -UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> -
> -//
> -// Set default value to assume MTRRs need to be configured on each SMI
> -//
> -BOOLEAN  mNeedConfigureMtrrs = TRUE;
> -
> -//
> -// Array for state of SMRR enable on all CPUs
> -//
> -BOOLEAN  *mSmrrEnabled;
> -
>  /**
>The constructor function
> 
> @@ -83,91 +36,9 @@ SmmCpuFeaturesLibConstructor (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> -  UINT32  RegEax;
> -  UINT32  RegEdx;
> -  UINTN   FamilyId;
> -  UINTN   ModelId;
> -
>//
> -  // Retrieve CPU Family and Model
> +  // No need to program SMRRs on our virtual platform.
>//
> -  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> -  FamilyId = (RegEax >> 8) & 0xf;
> -  ModelId  = (RegEax >> 4) & 0xf;
> -  if (FamilyId == 0x06 || FamilyId == 0x0f) {
> -ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> -  }
> -
> -  //
> -  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
> -  //
> -  if ((RegEdx & BIT12) != 0) {
> -//
> -// Check MTRR_CAP MSR bit 11 for SMRR support
> -//
> -if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
> -  mSmrrSupported = TRUE;
> -}
> -  }
> -
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
> -  //
> -  // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then
> -  // SMRR Physical Base and SMM Physical Mask MSRs are not available.
> -  //
> -  if (FamilyId == 0x06) {
> -if (ModelId == 0x1C || ModelId == 0x26 || 

Re: [edk2] [PATCH v5 21/33] OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg

[Kinney, Michael D]

Reviewed-by: Michael Kinney 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, November 26, 2015 6:35 PM
> To: edk2-de...@ml01.01.org
> Cc: Paolo Bonzini 
> Subject: [edk2] [PATCH v5 21/33] OvmfPkg: import SmmCpuFeaturesLib from 
> UefiCpuPkg
> 
> From: Paolo Bonzini 
> 
> The next patches will customize the implementation, but let's start from
> the common version to better show the changes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 
> [ler...@redhat.com: drop UNI file, keep whitespace intact, generate new
>  FILE_GUID, split off DSC changes, reflow commit message, refresh against
>  SVN r18958]
> Cc: Paolo Bonzini 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> v5:
> - refresh patch against SVN r18958 ("Add 2 APIs in SmmCpuFeaturesLib")
> - drop Mike's R-b consequently
> 
> v3:
> - new in v3
> 
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |  39 ++
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 658 
> 
>  2 files changed, 697 insertions(+)
> 
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> new file mode 100644
> index 000..deb08bf
> --- /dev/null
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -0,0 +1,39 @@
> +## @file
> +#  The CPU specific programming for PiSmmCpuDxeSmm module.
> +#
> +#  Copyright (c) 2009 - 2015, 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
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x00010005
> +  BASE_NAME  = SmmCpuFeaturesLib
> +  MODULE_UNI_FILE= SmmCpuFeaturesLib.uni
> +  FILE_GUID  = AC9991BE-D77A-464C-A8DE-A873DB8A4836
> +  MODULE_TYPE= DXE_SMM_DRIVER
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = SmmCpuFeaturesLib
> +  CONSTRUCTOR= SmmCpuFeaturesLibConstructor
> +
> +[Sources]
> +  SmmCpuFeaturesLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  PcdLib
> +  MemoryAllocationLib
> +  DebugLib
> +
> +[Pcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber## 
> SOMETIMES_CONSUMES
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> new file mode 100644
> index 000..3e480e1
> --- /dev/null
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -0,0 +1,658 @@
> +/** @file
> +The CPU specific programming for PiSmmCpuDxeSmm module.
> +
> +Copyright (c) 2010 - 2015, 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
> +which accompanies this distribution.  The full text of the license may be 
> found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +//
> +// Machine Specific Registers (MSRs)
> +//
> +#define  SMM_FEATURES_LIB_IA32_MTRR_CAP0x0FE
> +#define  SMM_FEATURES_LIB_IA32_FEATURE_CONTROL 0x03A
> +#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE   0x1F2
> +#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK   0x1F3
> +#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE  0x0A0
> +#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK  0x0A1
> +#defineEFI_MSR_SMRR_MASK   0xF000
> +#defineEFI_MSR_SMRR_PHYS_MASK_VALIDBIT11
> +#define  SMM_FEATURES_LIB_SMM_FEATURE_CONTROL  0x4E0
> +
> +//
> +// MSRs required for configuration of SMM Code Access Check
> +//
> +#define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
> +#define   SMM_CODE_ACCESS_CHK_BIT  BIT58
> +
> +//
> +// Set default value to assume SMRR is not supported
> +//
> +BOOLEAN  mSmrrSupported = FALSE;
> +
> +//
> +// Set default value to assume MSR_SMM_FEATURE_CONTROL is not supported
> +//
> +BOOLEAN  mSmmFeatureControlSupported = FALSE;
> +
> +//
> +// Set default value to assume IA-32 Architectural

Re: [edk2] EDK python script

[Andrew Fish]

> On Nov 26, 2015, at 9:26 AM, Laszlo Ersek  wrote:
> 
> On 11/26/15 04:45, Lu, James wrote:
>> 
>> Hi,
>> 
>> I'm trying to build EDK BIOS with python script on Linux but got below error.
>> 
>> [16:50:59]W:  [Step 2/5] Build environment: 
>> Linux-3.2.0-69-generic-x86_64-with-Ubuntu-12.04-precise
>> [16:50:59]W:  [Step 2/5] Build start time: 16:50:59, Nov.25 2015
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5] build.py...
>> [16:50:59]W:  [Step 2/5]  : error C0DE: Unknown fatal error when 
>> processing []
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5] (Please send email to 
>> edk2-de...@lists.sourceforge.net 
>> for help, attaching following call stack trace!)
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5] (Python 2.7.3 on linux2) Traceback (most recent 
>> call last):
>> [16:50:59]W:  [Step 2/5]   File 
>> "/opt/TCAgent/work/ee2ead8eb710175b/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py",
>>  line 2031, in Main
>> [16:50:59]W:  [Step 2/5] MyBuild = Build(Target, Workspace, Option)
>> [16:50:59]W:  [Step 2/5]   File 
>> "/opt/TCAgent/work/ee2ead8eb710175b/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py",
>>  line 761, in __init__
>> [16:50:59]W:  [Step 2/5] self.Db = 
>> WorkspaceDatabase(GlobalData.gDatabasePath, self.Reparse)
>> [16:50:59]W:  [Step 2/5]   File 
>> "/opt/TCAgent/work/ee2ead8eb710175b/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py",
>>  line 2814, in __init__
>> [16:50:59]W:  [Step 2/5] self.Conn.execute("PRAGMA synchronous=OFF")
>> [16:50:59]W:  [Step 2/5] DatabaseError: database disk image is malformed

Try removing Conf/.cache/ as it is the default location for the build database. 

Thanks,

Andrew Fish

>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5]
>> [16:50:59]W:  [Step 2/5] - Failed -
>> [16:50:59]W:  [Step 2/5] Build end time: 16:50:59, Nov.25 2015
>> [16:50:59]W:  [Step 2/5] Build total time: 00:00:00
>> 
>> Can you help?
> 
> After you provide the full build command line and the full output in
> your next message, I'm unsure if I will be able to help.
> 
> However, with the above truncated output, and no command line, I'm sure
> I can't help.
> 
> 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

[edk2] [PATCH] ArmPkg/ArmSoftfloatLib: add missing entry points for RVCT

[Ard Biesheuvel]

The RVCT compiler may emit calls to the various __aeabi_c?cmp??
functions, which return their results via the CPU condition flags
C and Z. According to ARM doc IHI 0043D 'Run-time ABI for the ARM
architecture':

The 3-way comparison functions c*cmple, c*cmpeq and c*rcmple return
their results in the CPSR Z and C flags. C is clear only if the operands
are ordered and the first operand is less than the second. Z is set only
when the operands are ordered and equal.

Add implementations for the double and float variants of the above.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cdcmp.asm | 54 
 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cfcmp.asm | 50 ++
 ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf   |  3 ++
 3 files changed, 107 insertions(+)

diff --git a/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cdcmp.asm 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cdcmp.asm
new file mode 100644
index ..1cc5b83d0cf6
--- /dev/null
+++ b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cdcmp.asm
@@ -0,0 +1,54 @@
+//--
+//
+// Copyright (c) 2015, Linaro Limited. 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
+// http://opensource.org/licenses/bsd-license.php
+//
+// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+//
+//--
+
+EXPORT  __aeabi_cdrcmple
+EXPORT  __aeabi_cdcmpeq
+EXPORT  __aeabi_cdcmple
+IMPORT  float64_eq
+IMPORT  float64_lt
+
+AREA__aeabi_cdcmp, CODE, READONLY
+
+CPSR_C_BIT  EQU   (1 << 29)
+CPSR_Z_BIT  EQU   (1 << 30)
+
+__aeabi_cdrcmple
+MOV IP, R0
+MOV R0, R2
+MOV R2, IP
+
+MOV IP, R1
+MOV R1, R3
+MOV R3, IP
+
+__aeabi_cdcmpeq
+__aeabi_cdcmple
+PUSH{LR}
+PUSH{R0 - R3}
+BL  float64_eq
+TEQ R0, #0
+BEQ NotEqual
+MSR CPSR_f, #CPSR_C_BIT :OR: CPSR_Z_BIT
+ADD SP, SP, #0x10
+POP {PC}
+
+NotEqual
+POP {R0 - R3}
+BL  float64_lt
+TEQ R0, #0
+MSREQ   CPSR_f, #CPSR_C_BIT
+MSRNE   CPSR_f, #0
+POP {PC}
+
+END
diff --git a/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cfcmp.asm 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cfcmp.asm
new file mode 100644
index ..f50b91de15a1
--- /dev/null
+++ b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_cfcmp.asm
@@ -0,0 +1,50 @@
+//--
+//
+// Copyright (c) 2015, Linaro Limited. 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
+// http://opensource.org/licenses/bsd-license.php
+//
+// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+//
+//--
+
+EXPORT  __aeabi_cfrcmple
+EXPORT  __aeabi_cfcmpeq
+EXPORT  __aeabi_cfcmple
+IMPORT  float32_eq
+IMPORT  float32_lt
+
+AREA__aeabi_cfcmp, CODE, READONLY
+
+CPSR_C_BIT  EQU   (1 << 29)
+CPSR_Z_BIT  EQU   (1 << 30)
+
+__aeabi_cfrcmple
+MOV IP, R0
+MOV R0, R1
+MOV R1, IP
+
+__aeabi_cfcmpeq
+__aeabi_cfcmple
+PUSH{LR}
+PUSH{R0 - R1}
+BL  float32_eq
+TEQ R0, #0
+BEQ NotEqual
+MSR CPSR_f, #CPSR_C_BIT :OR: CPSR_Z_BIT
+ADD SP, SP, #0x8
+POP {PC}
+
+NotEqual
+POP {R0 - R1}
+BL  float32_lt
+TEQ R0, #0
+MSREQ   CPSR_f, #CPSR_C_BIT
+MSRNE   CPSR_f, #0
+POP {PC}
+
+END
diff --git a/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf 
b/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
index 39c74bf1a3c2..3d3445197f49 100644
--- a/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
+++ b/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
@@ -41,6 +41,9 @@ [Sources]
   Arm/__aeabi_dcmpun.c
   Arm/__aeabi_fcmpun.c
 
+  Arm/__aeabi_cdcmp.asm   | RVCT
+  Arm/__aeabi_cfcmp.asm   | RVCT
+
 [Packages]
   MdePkg/MdePkg.dec
 
-- 
1.9.1

_

Re: [edk2] [PATCH v2 2/4] ArmPkg/ArmSoftFloatLib: add support for RVCT

[Ard Biesheuvel]

On 30 November 2015 at 14:49, Cohen, Eugene  wrote:
>> Yes. Unfortunately, OpenSslLib does not build at all for me with RVCT,
>> and I get many more errors than Eugene has reported, afaict.
>
> I believe I was still building with VFP floating point enabled - even though 
> the switch was removed from the OpensslLib.inf, my platform was still 
> defining VFP float in its dsc file (closed system where I know hard floats 
> are oaky).  Because of this approach I did not uncover the errors you were 
> seeing and as such my fix was incomplete, apologies.
>
> Thanks for finishing this.
>

No problem.

I tried to reproduce the issues when building OpensslLib from the
CryptoPkg .DSC but the results are quite different there. As it turns
out, CryptoPkg.dsc defines some RVCT flags that will probably need to
be moved to OpensslLib.inf.

But more disturbingly, I also found out that ArmSoftfloatLib (which is
tailored to GCC) lacks some functionality that RVCT apparently depends
on. The build of md_rand.obj (which is the only trivial user of FP in
the whole of OpenSSL, as I pointed out before) contains the following
__aeabi symbol references

 U __aeabi_cdcmple
 U __aeabi_cdrcmple
 U __aeabi_dadd
 U __aeabi_drsub
 U __aeabi_i2d


where the first two refer two softfloat functions that are not
implemented for GCC, i.e., double compare functions that return their
result in the CPU condition flags. This shouldn't be too hard to
implement, but it does take some effort.




> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Saturday, November 28, 2015 3:17 AM
> To: Ard Biesheuvel 
> Cc: edk2-devel@lists.01.org; Laszlo Ersek ; Cohen, Eugene 
> ; qlong 
> Subject: Re: [PATCH v2 2/4] ArmPkg/ArmSoftFloatLib: add support for RVCT
>
> On Fri, Nov 27, 2015 at 05:57:24PM +0100, Ard Biesheuvel wrote:
>> On 27 November 2015 at 17:09, Leif Lindholm  wrote:
>> > On Fri, Nov 27, 2015 at 04:21:55PM +0100, Ard Biesheuvel wrote:
>> >> The ARM softfloat library in ArmSoftfloatLib currently does not build
>> >> under RVCT, simply because the code includes system header files that
>> >> RVCT does not provide. However, nothing exported by those include files
>> >> is actually used by the library when built in SOFTFLOAT_FOR_GCC mode,
>> >> so we can just drop all of them.
>> >
>> > Looks plausible. If you can confirm you've tested it as well:
>> > Reviewed-by: Leif Lindholm 
>> >
>>
>> Yes. Unfortunately, OpenSslLib does not build at all for me with RVCT,
>> and I get many more errors than Eugene has reported, afaict.
>> But adding 'double foo(double a, double b) { return a * b; }' to any
>> other module and adding the ArmSoftfloatLib dependency to its .inf
>> shows that it builds the softfloat code, pulls it in and uses it to
>> resolve the various __aeabi_* symbols that the compiler emits when
>> running in softfloat mode.
>>
>> I should note that the *only* dependency for floating point I could
>> spot in OpenSSL (at least, in the part that we depend on) is a global
>> 'entropy' value in the random number generator that keeps track of the
>> minimum estimated entropy collected in the entropy pool. This means
>> the FP arithmetic that we perform is fairly trivial (adding and
>> comparing) so we'd need to mock things up substantially to validate
>> the softfloat functionality in its entirety.
>>
>> But I can confirm that this patch brings RVCT to the same level of
>> functionality as GCC, so if that is good enough for you, I'll gladly
>> take your R-b
>
> Given the circumstances, that's certainly good enough for me - thanks!
> If Eugene finds any issues building it with his setup, we can fix it
> up later.
>
> /
> Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/3] ArmPkg: factor out softfloat support from StdLib/LibC/SoftFloat

[Cohen, Eugene]

> I am aware of the CompilerIntrinsicsLib, but I have a slightly
> different point of view. The CompilerIntrinsicsLib needs to be added
> as a NULL library class resolution since the compiler may at any time
> generate calls to any of the functions it provides. In the case of
> software floating point, I'd actually prefer it to be explicit: once
> we add softfloat code to CompilerIntrinsicsLib, the cat is out of the
> bag, and we essentially add unlimited support to floating point
> anywhere in the code base. Note that AARCH64 will not require the
> same
> treatment, since the UEFI spec mandates support for hardware floating
> point in that case.

There are two ways to look at this:

1. Floating point is so special that we want everyone using it to make an 
explicit acknowledgement of it in the .inf file (but only on the ARM 
architecture which puts it at risk for breaking more often)
2. The compiler/architecture combination needs help from time to time - 
specifically what it needs help may vary across architectures and compilers.  
In this case is happens to be "just" floating point.

I don't know of a convention across all of edk2 for this but my inclination is 
to have a consistent treatment across all architectures - either burden them 
all with acknowledging floating point or make it all just work behind the 
scenes.

One idea: If the concern is abuse of floating point (code size / performance) 
then I share that concern - this could be handled by library classing where the 
default compilerintrinsics library does not implement floating point helpers 
and platforms are required to include the one with floating point support as a 
library class override.  It shifts the burden from the component to the 
platform but in this case I think it's the platform person that's more paranoid 
about floating point sneaking in anyways.  It doesn't scale well but if you 
only have a couple floating point enabled components anyways it's not too hard 
to maintain.

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

Re: [edk2] [PATCH 1/3] ArmPkg: factor out softfloat support from StdLib/LibC/SoftFloat

[Ard Biesheuvel]

On 30 November 2015 at 14:42, Cohen, Eugene  wrote:
> Ard,
>
> If these calls are compiler generated, then I think we have a library class 
> for this already: CompilerIntrinsicsLib.  As you can see in the ArmPkg 
> instance we already have the integer helper functions here.
>
> I prefer this approach because it's more manageable cross-architecture - 
> instead of an ARM-specific ArmSoftFloatLib you can just include 
> CompilerIntrinsicsLib and let the architecture-specific library selection 
> process do the right thing, providing whatever architecture/compiler help is 
> needed.
>

I am aware of the CompilerIntrinsicsLib, but I have a slightly
different point of view. The CompilerIntrinsicsLib needs to be added
as a NULL library class resolution since the compiler may at any time
generate calls to any of the functions it provides. In the case of
software floating point, I'd actually prefer it to be explicit: once
we add softfloat code to CompilerIntrinsicsLib, the cat is out of the
bag, and we essentially add unlimited support to floating point
anywhere in the code base. Note that AARCH64 will not require the same
treatment, since the UEFI spec mandates support for hardware floating
point in that case.


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Friday, November 27, 2015 1:52 AM
> To: edk2-devel@lists.01.org; leif.lindh...@linaro.org; ler...@redhat.com; 
> Cohen, Eugene ; qin.l...@intel.com
> Cc: Ard Biesheuvel 
> Subject: [PATCH 1/3] ArmPkg: factor out softfloat support from 
> StdLib/LibC/SoftFloat
>
> In order to support software floating point in the context of
> DXE drivers etc, this factors out the core ARM softfloat support
> into a separate library.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpeq.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpge.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpgt.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmple.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmplt.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpun.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpeq.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpge.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpgt.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmple.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmplt.c|  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpun.c|  0
>  {StdLib/Include => ArmPkg/Library/ArmSoftFloatLib}/Arm/softfloat.h   
>  | 33 +-
>  ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf   
>  | 48 
>  {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/arm-gcc.h 
>  |  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat-macros |  0
>  {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat.c 
>  |  0
>  {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/milieu.h  
>  |  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/softfloat-for-gcc.h |  0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/softfloat-specialize|  0
>  20 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpeq.c 
> b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c
> similarity index 100%
> copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpeq.c
> copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c
> diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpge.c 
> b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c
> similarity index 100%
> copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpge.c
> copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c
> diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpgt.c 
> b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c
> similarity index 100%
> copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpgt.c
> copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c
> diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmple.c 
> b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c
> similarity index 100%
> copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmple.c
> copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c
> diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmplt.c 
> b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmplt.c
> similarity index 100%
> copy from StdLib/LibC/Softfloat/Arm/__aeab

Re: [edk2] [PATCH v2 2/4] ArmPkg/ArmSoftFloatLib: add support for RVCT

[Cohen, Eugene]

> Yes. Unfortunately, OpenSslLib does not build at all for me with RVCT,
> and I get many more errors than Eugene has reported, afaict.

I believe I was still building with VFP floating point enabled - even though 
the switch was removed from the OpensslLib.inf, my platform was still defining 
VFP float in its dsc file (closed system where I know hard floats are oaky).  
Because of this approach I did not uncover the errors you were seeing and as 
such my fix was incomplete, apologies.

Thanks for finishing this.

Eugene

-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
Sent: Saturday, November 28, 2015 3:17 AM
To: Ard Biesheuvel 
Cc: edk2-devel@lists.01.org; Laszlo Ersek ; Cohen, Eugene 
; qlong 
Subject: Re: [PATCH v2 2/4] ArmPkg/ArmSoftFloatLib: add support for RVCT

On Fri, Nov 27, 2015 at 05:57:24PM +0100, Ard Biesheuvel wrote:
> On 27 November 2015 at 17:09, Leif Lindholm  wrote:
> > On Fri, Nov 27, 2015 at 04:21:55PM +0100, Ard Biesheuvel wrote:
> >> The ARM softfloat library in ArmSoftfloatLib currently does not build
> >> under RVCT, simply because the code includes system header files that
> >> RVCT does not provide. However, nothing exported by those include files
> >> is actually used by the library when built in SOFTFLOAT_FOR_GCC mode,
> >> so we can just drop all of them.
> >
> > Looks plausible. If you can confirm you've tested it as well:
> > Reviewed-by: Leif Lindholm 
> >
> 
> Yes. Unfortunately, OpenSslLib does not build at all for me with RVCT,
> and I get many more errors than Eugene has reported, afaict.
> But adding 'double foo(double a, double b) { return a * b; }' to any
> other module and adding the ArmSoftfloatLib dependency to its .inf
> shows that it builds the softfloat code, pulls it in and uses it to
> resolve the various __aeabi_* symbols that the compiler emits when
> running in softfloat mode.
> 
> I should note that the *only* dependency for floating point I could
> spot in OpenSSL (at least, in the part that we depend on) is a global
> 'entropy' value in the random number generator that keeps track of the
> minimum estimated entropy collected in the entropy pool. This means
> the FP arithmetic that we perform is fairly trivial (adding and
> comparing) so we'd need to mock things up substantially to validate
> the softfloat functionality in its entirety.
> 
> But I can confirm that this patch brings RVCT to the same level of
> functionality as GCC, so if that is good enough for you, I'll gladly
> take your R-b

Given the circumstances, that's certainly good enough for me - thanks!
If Eugene finds any issues building it with his setup, we can fix it
up later.

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

Re: [edk2] [PATCH 1/3] ArmPkg: factor out softfloat support from StdLib/LibC/SoftFloat

[Cohen, Eugene]

Ard,

If these calls are compiler generated, then I think we have a library class for 
this already: CompilerIntrinsicsLib.  As you can see in the ArmPkg instance we 
already have the integer helper functions here.

I prefer this approach because it's more manageable cross-architecture - 
instead of an ARM-specific ArmSoftFloatLib you can just include 
CompilerIntrinsicsLib and let the architecture-specific library selection 
process do the right thing, providing whatever architecture/compiler help is 
needed.

Thanks,

Eugene

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, November 27, 2015 1:52 AM
To: edk2-devel@lists.01.org; leif.lindh...@linaro.org; ler...@redhat.com; 
Cohen, Eugene ; qin.l...@intel.com
Cc: Ard Biesheuvel 
Subject: [PATCH 1/3] ArmPkg: factor out softfloat support from 
StdLib/LibC/SoftFloat

In order to support software floating point in the context of
DXE drivers etc, this factors out the core ARM softfloat support
into a separate library.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpeq.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpge.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpgt.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmple.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmplt.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_dcmpun.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpeq.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpge.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpgt.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmple.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmplt.c 
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/Arm/__aeabi_fcmpun.c 
   |  0
 {StdLib/Include => ArmPkg/Library/ArmSoftFloatLib}/Arm/softfloat.h 
   | 33 +-
 ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf 
   | 48 
 {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/arm-gcc.h   
   |  0
 {StdLib/LibC/Softfloat => 
ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat-macros |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat.c   
   |  0
 {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/milieu.h
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/softfloat-for-gcc.h  
   |  0
 {StdLib/LibC/Softfloat => ArmPkg/Library/ArmSoftFloatLib}/softfloat-specialize 
   |  0
 20 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpeq.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpeq.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpge.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpge.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpgt.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpgt.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmple.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmple.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmplt.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmplt.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmplt.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmplt.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_dcmpun.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpun.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_dcmpun.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpun.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_fcmpeq.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpeq.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_fcmpeq.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpeq.c
diff --git a/StdLib/LibC/Softfloat/Arm/__aeabi_fcmpge.c 
b/ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpge.c
similarity index 100%
copy from StdLib/LibC/Softfloat/Arm/__aeabi_fcmpge.c
copy to ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpge.c
diff --git a/StdLib/LibC/Softfloat/Arm/__ae

[edk2] [PATCH v2 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

From: Shannon Zhao 

Here we add the memory space for the memory nodes except the lowest one
in FDT. So these spaces will show up in the UEFI memory map.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Shannon Zhao 
---
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 72 
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 ++
 2 files changed, 76 insertions(+)

diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
index 74f80d1..11a9b5a 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -274,6 +275,73 @@ ProcessPciHost (
   return EFI_SUCCESS;
 }
 
+/**
+  Process the device tree node whose "device_type" property is "memory" and add
+the memory range of this node to System RAM space.
+
+  param[in] DeviceTreeBase  Pointer to the device tree.
+
+  param[in] NodeOffset of the device tree node.
+
+  @retval TRUE  The "device_type" property of this device tree node
+  is "memory" either adding successfully or
+  unsuccessfully.
+
+  @retval FALSE The "device_type" property of this device tree node
+  is not "memory"
+**/
+STATIC
+BOOLEAN
+EFIAPI
+AddMemorySpaceToSystemRam (
+  IN CONST VOID *DeviceTreeBase,
+  IN INT32  Node
+  )
+{
+  CONST CHAR8  *Type;
+  INT32Len;
+  CONST VOID   *RegProp;
+  EFI_STATUS   Status;
+  UINT64   CurBase;
+  UINT64   CurSize;
+
+  //
+  // Check for memory node and add the memory spaces expect the lowest one
+  //
+  Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
+  if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
+//
+// Get the 'reg' property of this node. For now, we will assume
+// two 8 byte quantities for base and size, respectively.
+//
+RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
+if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
+
+  CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
+  CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
+
+  if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
+Status = gDS->AddMemorySpace (
+EfiGcdMemoryTypeSystemMemory,
+CurBase, CurSize,
+EFI_MEMORY_WB | EFI_MEMORY_WC |
+EFI_MEMORY_WT | EFI_MEMORY_UC);
+
+if (EFI_ERROR (Status)) {
+  DEBUG ((EFI_D_ERROR, "%a: Failed to add System RAM @ 0x%lx - 
0x%lx\n",
+__FUNCTION__, CurBase, CurBase + CurSize - 1));
+} else {
+  DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
+__FUNCTION__, CurBase, CurBase + CurSize - 1));
+}
+  }
+}
+
+return TRUE;
+  }
+
+  return FALSE;
+}
 
 EFI_STATUS
 EFIAPI
@@ -332,6 +400,10 @@ InitializeVirtFdtDxe (
   break;
 }
 
+if (AddMemorySpaceToSystemRam (DeviceTreeBase, Node)) {
+  continue;
+}
+
 Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
 if (Type == NULL) {
   continue;
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf 
b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index ee2503a..0e6927b 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -43,12 +43,16 @@
   VirtioMmioDeviceLib
   HobLib
   XenIoMmioLib
+  DxeServicesTableLib
 
 [Guids]
   gFdtTableGuid
   gVirtioMmioTransportGuid
   gFdtHobGuid
 
+[FixedPcd]
+  gArmTokenSpaceGuid.PcdSystemMemoryBase
+
 [Pcd]
   gArmVirtTokenSpaceGuid.PcdArmPsciMethod
   gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
-- 
2.0.4


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

[edk2] [PATCH v2 1/2] ArmVirtPkg: Find the lowest memory node

[Shannon Zhao]

From: Shannon Zhao 

While QEMU NUMA support on ARM will introduce more than one /memory node
in the device tree, it needs to find the lowest one and set
PcdSystemMemorySize with the actual size of this memory node.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Shannon Zhao 
Reviewed-by: Laszlo Ersek 
---
 ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 30 
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c 
b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
index 17f2686..7a0fc0e 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
@@ -72,8 +72,8 @@ ArmPlatformInitializeSystemMemory (
 {
   VOID *DeviceTreeBase;
   INT32Node, Prev;
-  UINT64   NewBase;
-  UINT64   NewSize;
+  UINT64   NewBase, CurBase;
+  UINT64   NewSize, CurSize;
   CONST CHAR8  *Type;
   INT32Len;
   CONST UINT64 *RegProp;
@@ -90,7 +90,7 @@ ArmPlatformInitializeSystemMemory (
   ASSERT (fdt_check_header (DeviceTreeBase) == 0);
 
   //
-  // Look for a memory node
+  // Look for the lowest memory node
   //
   for (Prev = 0;; Prev = Node) {
 Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
@@ -110,26 +110,30 @@ ArmPlatformInitializeSystemMemory (
   RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
   if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
 
-NewBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
-NewSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
-
-//
-// Make sure the start of DRAM matches our expectation
-//
-ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
-PcdSet64 (PcdSystemMemorySize, NewSize);
+CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
+CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
 
 DEBUG ((EFI_D_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
-   __FUNCTION__, NewBase, NewBase + NewSize - 1));
+   __FUNCTION__, CurBase, CurBase + CurSize - 1));
+
+if (NewBase > CurBase || NewBase == 0) {
+  NewBase = CurBase;
+  NewSize = CurSize;
+}
   } else {
 DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
__FUNCTION__));
   }
-  break;
 }
   }
 
   //
+  // Make sure the start of DRAM matches our expectation
+  //
+  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
+  PcdSet64 (PcdSystemMemorySize, NewSize);
+
+  //
   // We need to make sure that the machine we are running on has at least
   // 128 MB of memory configured, and is currently executing this binary from
   // NOR flash. This prevents a device tree image in DRAM from getting
-- 
2.0.4


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

[edk2] [PATCH v2 0/2] ArmVirtPkg: Add support for multiple memory nodes

[Shannon Zhao]

From: Shannon Zhao 

If there are more than one memory nodes in FDT, currently UEFI will
assert. These two patches firstly let UEFI find the memory node with
lowest base address and check if the address is what we expected and set
PcdSystemMemorySize as the size of this node. Then add other memory
spaces through gDS->AddMemorySpace() when it parses FDT.

Changes since v1:
* Fix commit message and add reviewed-by tag from Laszlo (PATCH 1/2)
* Factor the codes out as a new function, fix coding style (PATCH 2/2)

Shannon Zhao (2):
  ArmVirtPkg: Find the lowest memory node
  ArmVirtPkg: Add memory space for the memory nodes except the lowest
one

 ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 30 +++-
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 72 
 ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 ++
 3 files changed, 93 insertions(+), 13 deletions(-)

-- 
2.0.4


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

Re: [edk2] [PATCH 0/2] ArmVirtPkg: limit IPA space to prevent page table memory waste

[Julien Grall]

(CCing Ian Campbell)

On 21/11/15 09:44, Ard Biesheuvel wrote:
> Hello all,

Hi Ard,

> 
> After Drew pointed out that mach-virt now populates the memory region beyond
> DRAM, I am proposing this approach instead. Since KVM limits its IPA space to
> 40 bits, there is simply no point in supporting anything beyond that for
> ArmVirtQemu.
> 
> I am cc'ing the Xen guys since they may run into similar issues on ThunderX, 
> or
> any other hardware whose support PA space is so large. Their platform does not
> use ArmVirtPkg/ArmVirtPlatformLib though, so a similar change may be necssary 
> on
> the Xen end.

There is no fixed limit for the PA space on Xen as we re-use the one
provided by the hardware.

Regards,

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

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
> On 11/30/15 11:03, Ard Biesheuvel wrote:
[...]
>>
>> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
>> HOBs the first time we enumerate the nodes?
>
> I didn't suggest it for two reasons.
>
> (1) I recalled that last time we had had a very long discussion about how the 
> DXE core selects the initial range for memory allocations (for its own 
> purposes, primarily). I had trouble remembering all the details now. So there 
> were three options for me:
>
> - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE core 
> picks will be good for us
>
> - recommend the HOBs, and actually review what the DXE core does (it was 
> modified a little bit last time)
>
> - recommend a single HOB (which implies the DXE core gets initialized exactly 
> the same as before, no thinking or code browsing needed), and delay the 
> installation of the addtional ranges until later.
>
> I picked the last option for simplicity.
>
> (2) The other reason is that I don't think that the HOB approach would solve 
> the question of caching attributes. I don't think the DXE core tries to set 
> any attributes on its own when the GCD is primed from the HOBs.
>
> Remember we have
>
> InitializeMemory()  
> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>   ArmPlatformInitializeSystemMemory()   
> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
> PcdSet64 (PcdSystemMemorySize, ...)
>   MemoryPeim()  
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
> PcdGet64 (PcdSystemMemorySize)
> BuildResourceDescriptorHob()
> InitMmu()   
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>   ArmPlatformGetVirtualMemoryMap()  
> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
> PcdGet64 (PcdSystemMemorySize)
>   ArmConfigureMmu() 
> [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
>
> In other words, all of the following happens in the memory init PEIM (which 
> comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg 
> library instances):
> - the in-DRAM DTB is initially parsed for the memory node
> - the size PCD is set
> - the base PCD is verified
> - a memory descriptor HOB is built, dependent on the PCDs
> - a virtual memory map is built, with caching attributes, dependent on the 
> PCDs
> - the MMU is configured
>
> Therefore we have a nice fast caching setting for the "base" memory node only 
> because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() -- 
> dependent on the PCDs --, not because the DXE core covers it when it 
> processes the HOBs. So if you want to process several memory nodes *for real* 
> in the the memory init PEIM, then not only do you have to create the HOBs 
> there, but also extend the virtual memory map for the MMU similarly. And a 
> fixed count of PCDs won't be enough to carry the information from the DTB to 
> ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist that 
> connects the DTB with the virtual memory map.
>
> This is what I meant in my original response by "having more flexibility in 
> DXE".
>

Yes, that does sound like a lot of work for little gain. But I am not
too crazy about adding more 'A PRIORI' modules, to avoid ending up in
dependency hell later.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 12:06, Shannon Zhao wrote:
> 
> 
> On 2015/11/30 18:01, Laszlo Ersek wrote:
>> On 11/30/15 10:50, Laszlo Ersek wrote:
>>> On 11/30/15 10:28, Ard Biesheuvel wrote:
 On 30 November 2015 at 10:22, Shannon Zhao  
 wrote:
>
>
> On 2015/11/30 16:53, Laszlo Ersek wrote:
>> On 11/29/15 07:31, Shannon Zhao wrote:
>>> From: Shannon Zhao 
>>>
>>> Here we add the memory space for the memory nodes except the lowest one
>>> in FDT. So these spaces will show up in the UEFI memory map.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Shannon Zhao 
>>> ---
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
>>> ++
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>>  2 files changed, 38 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> index 74f80d1..2c8d785 100644
>>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> @@ -27,6 +27,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>UINT64 FwCfgDataSize;
>>>UINT64 FwCfgDmaAddress;
>>>UINT64 FwCfgDmaSize;
>>> +  UINT64 CurBase;
>>> +  UINT64 CurSize;
>>>
>>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>>break;
>>>  }
>>>
>>> +//
>>> +// Check for memory node and add the memory spaces expect the 
>>> lowest one
>>> +//
>>
>> (1) Based on the DTB node, which looks like:
>>
>> memory {
>> reg = <0x0 0x4000 0x0 0x800>;
>> device_type = "memory";
>> };
>>
>> I agree that checking it here, before we look at "compatible", is a good
>> thing.
>>
>> However, can you please factor this out into a separate function? (Just
>> within this file, as a STATIC function.)
>>
>> I propose that the function return a BOOLEAN:
>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>   in which case you can "continue;" with the next iteration directly
>> - FALSE, if there is no match (i.e., the current iteration must
>>   proceed, in order to look for different device types)
>>
>
> Sure.
>
>>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>>> +  //
>>> +  // Get the 'reg' property of this node. For now, we will assume
>>> +  // two 8 byte quantities for base and size, respectively.
>>> +  //
>>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>>> +
>>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>>> +
>>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>>> +   CurBase, CurSize,
>>> +   EFI_MEMORY_WB | 
>>> EFI_MEMORY_RUNTIME);
>>
>> (1) The edk2 coding style requires a space between the function name (or
>> function pointer) and the opening paren.
>>
>> (2) Regarding the indentation of function arguments, they are aligned
>> with the first or (more usually:) second character of the function or
>> *member* name. In this case, they should be aligned with the second "d"
>> in "AddMemorySpace".
>>
> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with 
> this.
>
>> (3) The last argument is a bitmask of capabilities. As far as I
>> remember, all system memory regions I've seen dumped by Linux from the
>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>
>> I agree that RUNTIME and WB should be listed here, but do you have any
>> particular reason for not allowing UC|WC|WT?
>>
>> (4) Related question -- when you boot the guest kernel with this patch
>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>> calls.)
>>
>> Can you paste a sample output from "efi=debug"?
>>
>
> Processing EFI memory map:
>   0x

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 11:03, Ard Biesheuvel wrote:
> On 30 November 2015 at 11:01, Laszlo Ersek  wrote:
>> On 11/30/15 10:50, Laszlo Ersek wrote:
>>> On 11/30/15 10:28, Ard Biesheuvel wrote:
 On 30 November 2015 at 10:22, Shannon Zhao  
 wrote:
>
>
> On 2015/11/30 16:53, Laszlo Ersek wrote:
>> On 11/29/15 07:31, Shannon Zhao wrote:
>>> From: Shannon Zhao 
>>>
>>> Here we add the memory space for the memory nodes except the lowest one
>>> in FDT. So these spaces will show up in the UEFI memory map.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Shannon Zhao 
>>> ---
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
>>> ++
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>>  2 files changed, 38 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> index 74f80d1..2c8d785 100644
>>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> @@ -27,6 +27,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>UINT64 FwCfgDataSize;
>>>UINT64 FwCfgDmaAddress;
>>>UINT64 FwCfgDmaSize;
>>> +  UINT64 CurBase;
>>> +  UINT64 CurSize;
>>>
>>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>>break;
>>>  }
>>>
>>> +//
>>> +// Check for memory node and add the memory spaces expect the 
>>> lowest one
>>> +//
>>
>> (1) Based on the DTB node, which looks like:
>>
>> memory {
>> reg = <0x0 0x4000 0x0 0x800>;
>> device_type = "memory";
>> };
>>
>> I agree that checking it here, before we look at "compatible", is a good
>> thing.
>>
>> However, can you please factor this out into a separate function? (Just
>> within this file, as a STATIC function.)
>>
>> I propose that the function return a BOOLEAN:
>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>   in which case you can "continue;" with the next iteration directly
>> - FALSE, if there is no match (i.e., the current iteration must
>>   proceed, in order to look for different device types)
>>
>
> Sure.
>
>>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>>> +  //
>>> +  // Get the 'reg' property of this node. For now, we will assume
>>> +  // two 8 byte quantities for base and size, respectively.
>>> +  //
>>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>>> +
>>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>>> +
>>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>>> +   CurBase, CurSize,
>>> +   EFI_MEMORY_WB | 
>>> EFI_MEMORY_RUNTIME);
>>
>> (1) The edk2 coding style requires a space between the function name (or
>> function pointer) and the opening paren.
>>
>> (2) Regarding the indentation of function arguments, they are aligned
>> with the first or (more usually:) second character of the function or
>> *member* name. In this case, they should be aligned with the second "d"
>> in "AddMemorySpace".
>>
> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with 
> this.
>
>> (3) The last argument is a bitmask of capabilities. As far as I
>> remember, all system memory regions I've seen dumped by Linux from the
>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>
>> I agree that RUNTIME and WB should be listed here, but do you have any
>> particular reason for not allowing UC|WC|WT?
>>
>> (4) Related question -- when you boot the guest kernel with this patch
>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>> calls.)
>>
>> Can you paste a sample output from "efi=debug"?
>>
>
> Processing EFI memory map:

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

On 2015/11/30 18:01, Laszlo Ersek wrote:
> On 11/30/15 10:50, Laszlo Ersek wrote:
>> On 11/30/15 10:28, Ard Biesheuvel wrote:
>>> On 30 November 2015 at 10:22, Shannon Zhao  wrote:


 On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Here we add the memory space for the memory nodes except the lowest one
>> in FDT. So these spaces will show up in the UEFI memory map.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Shannon Zhao 
>> ---
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
>> ++
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> index 74f80d1..2c8d785 100644
>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> @@ -27,6 +27,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>
>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>break;
>>  }
>>
>> +//
>> +// Check for memory node and add the memory spaces expect the 
>> lowest one
>> +//
>
> (1) Based on the DTB node, which looks like:
>
> memory {
> reg = <0x0 0x4000 0x0 0x800>;
> device_type = "memory";
> };
>
> I agree that checking it here, before we look at "compatible", is a good
> thing.
>
> However, can you please factor this out into a separate function? (Just
> within this file, as a STATIC function.)
>
> I propose that the function return a BOOLEAN:
> - TRUE if the node was a match (either successfully or unsuccessfully);
>   in which case you can "continue;" with the next iteration directly
> - FALSE, if there is no match (i.e., the current iteration must
>   proceed, in order to look for different device types)
>

 Sure.

>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>> +  //
>> +  // Get the 'reg' property of this node. For now, we will assume
>> +  // two 8 byte quantities for base and size, respectively.
>> +  //
>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>> +
>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>> +
>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>> +   CurBase, CurSize,
>> +   EFI_MEMORY_WB | 
>> EFI_MEMORY_RUNTIME);
>
> (1) The edk2 coding style requires a space between the function name (or
> function pointer) and the opening paren.
>
> (2) Regarding the indentation of function arguments, they are aligned
> with the first or (more usually:) second character of the function or
> *member* name. In this case, they should be aligned with the second "d"
> in "AddMemorySpace".
>
 Thanks for explain the edk2 coding style. Sorry, I'm not familiar with 
 this.

> (3) The last argument is a bitmask of capabilities. As far as I
> remember, all system memory regions I've seen dumped by Linux from the
> UEFI memmap at startup had all of: UC | WC | WT | WB.
>
> I agree that RUNTIME and WB should be listed here, but do you have any
> particular reason for not allowing UC|WC|WT?
>
> (4) Related question -- when you boot the guest kernel with this patch
> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
> of the additional NUMA nodes look identically to the "base" memory? (I'm
> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
> calls.)
>
> Can you paste a sample output from "efi=debug"?
>

 Processing EFI memory map:
   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
   |  |  |  |UC]
   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>>

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 30 November 2015 at 11:01, Laszlo Ersek  wrote:
> On 11/30/15 10:50, Laszlo Ersek wrote:
>> On 11/30/15 10:28, Ard Biesheuvel wrote:
>>> On 30 November 2015 at 10:22, Shannon Zhao  wrote:


 On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Here we add the memory space for the memory nodes except the lowest one
>> in FDT. So these spaces will show up in the UEFI memory map.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Shannon Zhao 
>> ---
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
>> ++
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> index 74f80d1..2c8d785 100644
>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> @@ -27,6 +27,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>
>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>break;
>>  }
>>
>> +//
>> +// Check for memory node and add the memory spaces expect the 
>> lowest one
>> +//
>
> (1) Based on the DTB node, which looks like:
>
> memory {
> reg = <0x0 0x4000 0x0 0x800>;
> device_type = "memory";
> };
>
> I agree that checking it here, before we look at "compatible", is a good
> thing.
>
> However, can you please factor this out into a separate function? (Just
> within this file, as a STATIC function.)
>
> I propose that the function return a BOOLEAN:
> - TRUE if the node was a match (either successfully or unsuccessfully);
>   in which case you can "continue;" with the next iteration directly
> - FALSE, if there is no match (i.e., the current iteration must
>   proceed, in order to look for different device types)
>

 Sure.

>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>> +  //
>> +  // Get the 'reg' property of this node. For now, we will assume
>> +  // two 8 byte quantities for base and size, respectively.
>> +  //
>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>> +
>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>> +
>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>> +   CurBase, CurSize,
>> +   EFI_MEMORY_WB | 
>> EFI_MEMORY_RUNTIME);
>
> (1) The edk2 coding style requires a space between the function name (or
> function pointer) and the opening paren.
>
> (2) Regarding the indentation of function arguments, they are aligned
> with the first or (more usually:) second character of the function or
> *member* name. In this case, they should be aligned with the second "d"
> in "AddMemorySpace".
>
 Thanks for explain the edk2 coding style. Sorry, I'm not familiar with 
 this.

> (3) The last argument is a bitmask of capabilities. As far as I
> remember, all system memory regions I've seen dumped by Linux from the
> UEFI memmap at startup had all of: UC | WC | WT | WB.
>
> I agree that RUNTIME and WB should be listed here, but do you have any
> particular reason for not allowing UC|WC|WT?
>
> (4) Related question -- when you boot the guest kernel with this patch
> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
> of the additional NUMA nodes look identically to the "base" memory? (I'm
> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
> calls.)
>
> Can you paste a sample output from "efi=debug"?
>

 Processing EFI memory map:
   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
   |  |  |  |UC]
   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 10:50, Laszlo Ersek wrote:
> On 11/30/15 10:28, Ard Biesheuvel wrote:
>> On 30 November 2015 at 10:22, Shannon Zhao  wrote:
>>>
>>>
>>> On 2015/11/30 16:53, Laszlo Ersek wrote:
 On 11/29/15 07:31, Shannon Zhao wrote:
> From: Shannon Zhao 
>
> Here we add the memory space for the memory nodes except the lowest one
> in FDT. So these spaces will show up in the UEFI memory map.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Shannon Zhao 
> ---
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
> ++
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>  2 files changed, 38 insertions(+)
>
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> index 74f80d1..2c8d785 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>UINT64 FwCfgDataSize;
>UINT64 FwCfgDmaAddress;
>UINT64 FwCfgDmaSize;
> +  UINT64 CurBase;
> +  UINT64 CurSize;
>
>Hob = GetFirstGuidHob(&gFdtHobGuid);
>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>break;
>  }
>
> +//
> +// Check for memory node and add the memory spaces expect the lowest 
> one
> +//

 (1) Based on the DTB node, which looks like:

 memory {
 reg = <0x0 0x4000 0x0 0x800>;
 device_type = "memory";
 };

 I agree that checking it here, before we look at "compatible", is a good
 thing.

 However, can you please factor this out into a separate function? (Just
 within this file, as a STATIC function.)

 I propose that the function return a BOOLEAN:
 - TRUE if the node was a match (either successfully or unsuccessfully);
   in which case you can "continue;" with the next iteration directly
 - FALSE, if there is no match (i.e., the current iteration must
   proceed, in order to look for different device types)

>>>
>>> Sure.
>>>
> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +  //
> +  // Get the 'reg' property of this node. For now, we will assume
> +  // two 8 byte quantities for base and size, respectively.
> +  //
> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
> +
> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
> +   CurBase, CurSize,
> +   EFI_MEMORY_WB | 
> EFI_MEMORY_RUNTIME);

 (1) The edk2 coding style requires a space between the function name (or
 function pointer) and the opening paren.

 (2) Regarding the indentation of function arguments, they are aligned
 with the first or (more usually:) second character of the function or
 *member* name. In this case, they should be aligned with the second "d"
 in "AddMemorySpace".

>>> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with this.
>>>
 (3) The last argument is a bitmask of capabilities. As far as I
 remember, all system memory regions I've seen dumped by Linux from the
 UEFI memmap at startup had all of: UC | WC | WT | WB.

 I agree that RUNTIME and WB should be listed here, but do you have any
 particular reason for not allowing UC|WC|WT?

 (4) Related question -- when you boot the guest kernel with this patch
 in the firmware, and pass "efi=debug" to Linux, do the memory attributes
 of the additional NUMA nodes look identically to the "base" memory? (I'm
 asking this to see if we need additional gDS->SetMemorySpaceAttributes()
 calls.)

 Can you paste a sample output from "efi=debug"?

>>>
>>> Processing EFI memory map:
>>>   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>>   |  |  |  |UC]
>>>   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>>   |  |  |  |UC]
>>>   0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
>>>   |WB|WT|WC|UC]
>>>   0x4001-0x4007 [Conventional Memory|

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 10:28, Ard Biesheuvel wrote:
> On 30 November 2015 at 10:22, Shannon Zhao  wrote:
>>
>>
>> On 2015/11/30 16:53, Laszlo Ersek wrote:
>>> On 11/29/15 07:31, Shannon Zhao wrote:
 From: Shannon Zhao 

 Here we add the memory space for the memory nodes except the lowest one
 in FDT. So these spaces will show up in the UEFI memory map.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Shannon Zhao 
 ---
  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
 ++
  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
  2 files changed, 38 insertions(+)

 diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
 b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
 index 74f80d1..2c8d785 100644
 --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
 +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
 @@ -27,6 +27,7 @@
  #include 
  #include 
  #include 
 +#include 

  #include 
  #include 
 @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
UINT64 FwCfgDataSize;
UINT64 FwCfgDmaAddress;
UINT64 FwCfgDmaSize;
 +  UINT64 CurBase;
 +  UINT64 CurSize;

Hob = GetFirstGuidHob(&gFdtHobGuid);
if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
 @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
break;
  }

 +//
 +// Check for memory node and add the memory spaces expect the lowest 
 one
 +//
>>>
>>> (1) Based on the DTB node, which looks like:
>>>
>>> memory {
>>> reg = <0x0 0x4000 0x0 0x800>;
>>> device_type = "memory";
>>> };
>>>
>>> I agree that checking it here, before we look at "compatible", is a good
>>> thing.
>>>
>>> However, can you please factor this out into a separate function? (Just
>>> within this file, as a STATIC function.)
>>>
>>> I propose that the function return a BOOLEAN:
>>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>>   in which case you can "continue;" with the next iteration directly
>>> - FALSE, if there is no match (i.e., the current iteration must
>>>   proceed, in order to look for different device types)
>>>
>>
>> Sure.
>>
 +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
 +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
 +  //
 +  // Get the 'reg' property of this node. For now, we will assume
 +  // two 8 byte quantities for base and size, respectively.
 +  //
 +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
 +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
 +
 +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
 +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
 +
 +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
 +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
 +   CurBase, CurSize,
 +   EFI_MEMORY_WB | 
 EFI_MEMORY_RUNTIME);
>>>
>>> (1) The edk2 coding style requires a space between the function name (or
>>> function pointer) and the opening paren.
>>>
>>> (2) Regarding the indentation of function arguments, they are aligned
>>> with the first or (more usually:) second character of the function or
>>> *member* name. In this case, they should be aligned with the second "d"
>>> in "AddMemorySpace".
>>>
>> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with this.
>>
>>> (3) The last argument is a bitmask of capabilities. As far as I
>>> remember, all system memory regions I've seen dumped by Linux from the
>>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>>
>>> I agree that RUNTIME and WB should be listed here, but do you have any
>>> particular reason for not allowing UC|WC|WT?
>>>
>>> (4) Related question -- when you boot the guest kernel with this patch
>>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>>> calls.)
>>>
>>> Can you paste a sample output from "efi=debug"?
>>>
>>
>> Processing EFI memory map:
>>   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>   |  |  |  |UC]
>>   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>   |  |  |  |UC]
>>   0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
>>   |WB|WT|WC|UC]
>>   0x4001-0x4007 [Conventional Memory|   |  |  |  |  |  |
>>   |WB|WT|WC|UC]
>>
>> [cut some information here]
>>
>>   0x6000-0xbfff [Conventional Memory|RUN|  |  |  |  |  |
>>   |WB|  |

Re: [edk2] Armv8 64bit: System error booting linux from the UEFI

[Michael Zimmermann]

> Unrelated note: *please* get rid of the ARM BDS and use the Intel BDS
> instead. The ARM BDS violates the UEFI spec, (i.e., you cannot boot
> installer images from it without a lot of hassle) and does not allow
> you to enable UEFI secure boot.
>
> Look at the development history of ArmVExpress-FVP-AArch64.dsc for
> hints how to do this.

To make this easier for other, here is my commit for switching to Intel's
BDS(my code is based on ArmPlatformPkg/ArmPlatformPkg-2ndstage):
https://github.com/efidroid/uefi_edk2packages_LittleKernelPkg/commit/dbbf212823046e00db3879c6846d96532f703e30

You may also need dynamic PCD support:
https://github.com/efidroid/uefi_edk2packages_LittleKernelPkg/commit/673cd3bc2d71bf077e5744d9de1597a356b6d2d5

On Tue, Nov 24, 2015 at 8:28 AM, Ard Biesheuvel 
wrote:

> On 24 November 2015 at 00:05, Vladimir Olovyannikov
>  wrote:
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: Wednesday, November 18, 2015 11:32 PM
> >> To: Vladimir Olovyannikov
> >> Cc: Mark Rutland; edk2-devel@lists.01.org
> >> Subject: Re: [edk2] Armv8 64bit: System error booting linux from the
> UEFI
> >>
> >> On 19 November 2015 at 05:48, Vladimir Olovyannikov
> >>  wrote:
> >> >
> >> >
> > [...]
> >> > A side note: I got the u-boot source for that board and there are
> several
> >> hacks made to avoid SError (writing 0x20 to the HCR register (reroute
> SError
> >> to EL2), and
> >> > just ERET from SError exception handler, and then write 0x0 to HCR
> before
> >> Linux boots), so it could be an HW issue I am not aware of as of yet.
> >>
> >> OK, that would explain it. Note that the kernel will replace the EL2
> >> vector table if booted at EL2, so this is definitely not a workaround
> >> that you would want to reuse in UEFI.
> > [...]
> >
> > [...]
> >> >>
> >> >> I'd still like to double check the value of VBAR_EL2 as it is
> written,
> >> >> it should be a multiple of 2 KB
> >> > It is always at 0x85008800 which is a multiple of 2K
> >> > Any other things to verify?
> >> >
> >>
> >> No, that looks fine. You need to get in touch with the authors of the
> >> U-Boot code to figure out what it is they are working around. Simply
> >> ignoring SErrors is obviously not a long term solution.
> >>
> > Thanks a lot for your help Ard, Mark,
> > It is the ATF which has an issue. I will post additional information if
> there is still a problem after ATF implementation is fixed
> >
>
> OK.
> Thanks for reporting back.
>
> --
> Ard.
> ___
> 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/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 30 November 2015 at 10:22, Shannon Zhao  wrote:
>
>
> On 2015/11/30 16:53, Laszlo Ersek wrote:
>> On 11/29/15 07:31, Shannon Zhao wrote:
>>> From: Shannon Zhao 
>>>
>>> Here we add the memory space for the memory nodes except the lowest one
>>> in FDT. So these spaces will show up in the UEFI memory map.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Shannon Zhao 
>>> ---
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 
>>> ++
>>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>>  2 files changed, 38 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> index 74f80d1..2c8d785 100644
>>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>> @@ -27,6 +27,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>UINT64 FwCfgDataSize;
>>>UINT64 FwCfgDmaAddress;
>>>UINT64 FwCfgDmaSize;
>>> +  UINT64 CurBase;
>>> +  UINT64 CurSize;
>>>
>>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>>break;
>>>  }
>>>
>>> +//
>>> +// Check for memory node and add the memory spaces expect the lowest 
>>> one
>>> +//
>>
>> (1) Based on the DTB node, which looks like:
>>
>> memory {
>> reg = <0x0 0x4000 0x0 0x800>;
>> device_type = "memory";
>> };
>>
>> I agree that checking it here, before we look at "compatible", is a good
>> thing.
>>
>> However, can you please factor this out into a separate function? (Just
>> within this file, as a STATIC function.)
>>
>> I propose that the function return a BOOLEAN:
>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>   in which case you can "continue;" with the next iteration directly
>> - FALSE, if there is no match (i.e., the current iteration must
>>   proceed, in order to look for different device types)
>>
>
> Sure.
>
>>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>>> +  //
>>> +  // Get the 'reg' property of this node. For now, we will assume
>>> +  // two 8 byte quantities for base and size, respectively.
>>> +  //
>>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>>> +
>>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>>> +
>>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>>> +   CurBase, CurSize,
>>> +   EFI_MEMORY_WB | EFI_MEMORY_RUNTIME);
>>
>> (1) The edk2 coding style requires a space between the function name (or
>> function pointer) and the opening paren.
>>
>> (2) Regarding the indentation of function arguments, they are aligned
>> with the first or (more usually:) second character of the function or
>> *member* name. In this case, they should be aligned with the second "d"
>> in "AddMemorySpace".
>>
> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with this.
>
>> (3) The last argument is a bitmask of capabilities. As far as I
>> remember, all system memory regions I've seen dumped by Linux from the
>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>
>> I agree that RUNTIME and WB should be listed here, but do you have any
>> particular reason for not allowing UC|WC|WT?
>>
>> (4) Related question -- when you boot the guest kernel with this patch
>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>> calls.)
>>
>> Can you paste a sample output from "efi=debug"?
>>
>
> Processing EFI memory map:
>   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>   |  |  |  |UC]
>   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>   |  |  |  |UC]
>   0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
>   |WB|WT|WC|UC]
>   0x4001-0x4007 [Conventional Memory|   |  |  |  |  |  |
>   |WB|WT|WC|UC]
>
> [cut some information here]
>
>   0x6000-0xbfff [Conventional Memory|RUN|  |  |  |  |  |
>   |WB|  |  |  ]
>
> Looks like the region is showed with the attributes we set. While
> comparing with other Conventional Memory, it doesn't have RUNTIME. So
> drop it?
>

You should no

Re: [edk2] GICv3 support

[Ard Biesheuvel]

On 30 November 2015 at 04:52, Meenakshi Aggarwal
 wrote:
> Hi,
>
>
> I have added support of GICv3 in my board package by defining
>
>   gArmTokenSpaceGuid.PcdGicDistributorBase|0x600
>   gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x610
>
>
> But still I am unable to take input from keyboard.
>
> Do I need to do some more settings for this.
>

You have to make sure the firmware in EL3 sets up the GIC correctly.
If you are using ARM Trusted Firmware *and* your GIC has v2
compatibility mode implemented, you can set

gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy

to TRUE in your .DSC file (and hope for the best)

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

[edk2] [Patch] BaseTools: Add VS2015 tool chain in tools_def.template

[Liming Gao]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Yu 
Reviewed-by: Liming Gao 
---
 BaseTools/Conf/tools_def.template | 519 +-
 edksetup.bat  |  38 +--
 2 files changed, 536 insertions(+), 21 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template 
b/BaseTools/Conf/tools_def.template
index db08e25..88d4b4f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -65,6 +65,14 @@ DEFINE VS2013x86_BIN= C:\Program Files (x86)\Microsoft 
Visual Studio 12.0\Vc
 DEFINE VS2013x86_DLL= C:\Program Files (x86)\Microsoft Visual Studio 
12.0\Common7\IDE;DEF(VS2013x86_BIN)
 DEFINE VS2013x86_BINX64 = DEF(VS2013x86_BIN)\x86_amd64
 
+DEFINE VS2015_BIN  = C:\Program Files\Microsoft Visual Studio 14.0\Vc\bin
+DEFINE VS2015_DLL  = C:\Program Files\Microsoft Visual Studio 
14.0\Common7\IDE;DEF(VS2015_BIN)
+DEFINE VS2015_BINX64   = DEF(VS2015_BIN)\x86_amd64
+
+DEFINE VS2015x86_BIN= C:\Program Files (x86)\Microsoft Visual Studio 
14.0\Vc\bin
+DEFINE VS2015x86_DLL= C:\Program Files (x86)\Microsoft Visual Studio 
14.0\Common7\IDE;DEF(VS2015x86_BIN)
+DEFINE VS2015x86_BINX64 = DEF(VS2015x86_BIN)\x86_amd64
+
 DEFINE WINSDK_VERSION   = v6.0A
 DEFINE WINSDK_BIN   = c:\Program Files\Microsoft 
SDKs\Windows\DEF(WINSDK_VERSION)\bin
 DEFINE WINSDKx86_BIN= c:\Program Files (x86)\Microsoft 
SDKs\Windows\DEF(WINSDK_VERSION)\bin
@@ -81,6 +89,10 @@ DEFINE WINSDK71x86_BIN= c:\Program Files (x86)\Microsoft 
SDKs\Windows\v7.1A\
 DEFINE WINSDK8_BIN   = c:\Program Files\Windows Kits\8.0\bin\x86\
 DEFINE WINSDK8x86_BIN= c:\Program Files (x86)\Windows Kits\8.0\bin\x64
 
+# Microsoft Visual Studio 2015 Professional Edition
+DEFINE WINSDK81_BIN   = c:\Program Files\Windows Kits\8.1\bin\x86\
+DEFINE WINSDK81x86_BIN= c:\Program Files (x86)\Windows Kits\8.1\bin\x64
+
 # These defines are needed for certain Microsoft Visual Studio tools that
 # are used by other toolchains.  An example is that ICC on Windows normally
 # uses Microsoft's nmake.exe.
@@ -293,6 +305,15 @@ DEFINE SOURCERY_CYGWIN_TOOLS = /cygdrive/c/Program 
Files/CodeSourcery/Sourcery G
 # Required to build platforms or ACPI tables:
 #   Intel(r) ACPI Compiler (iasl.exe) from
 #   https://acpica.org/downloads
+#   VS2015  -win32-  Requires:
+# Microsoft Visual Studio 2015 Professional Edition
+# Microsoft Windows Server 2003 Driver Development 
Kit (Microsoft WINDDK) version 3790.1830
+#Optional:
+# Required to build EBC drivers:
+#   Intel(r) Compiler for Efi Byte Code (Intel(r) 
EBC Compiler)
+# Required to build platforms or ACPI tables:
+#   Intel(r) ACPI Compiler (iasl.exe) from
+#   https://acpica.org/downloads
 #   DDK3790 -win32-  Requires:
 # Microsoft Windows Server 2003 Driver Development 
Kit (Microsoft WINDDK) version 3790.1830
 #Optional:
@@ -452,6 +473,15 @@ DEFINE SOURCERY_CYGWIN_TOOLS = /cygdrive/c/Program 
Files/CodeSourcery/Sourcery G
 # Required to build platforms or ACPI tables:
 #   Microsoft ASL ACPI Compiler (asl.exe) v4.0.0 
from
 #   
http://download.microsoft.com/download/2/c/1/2c16c7e0-96c1-40f5-81fc-3e4bf7b65496/microsoft_asl_compiler-v4-0-0.msi
+#   VS2015xASL  -win32-  Requires:
+# Microsoft Visual Studio 2015 Professional Edition
+# Microsoft Windows Server 2003 Driver Development 
Kit (Microsoft WINDDK) version 3790.1830
+#Optional:
+# Required to build EBC drivers:
+#   Intel(r) Compiler for Efi Byte Code (Intel(r) 
EBC Compiler)
+# Required to build platforms or ACPI tables:
+#   Microsoft ASL ACPI Compiler (asl.exe) v4.0.0 
from
+#   
http://download.microsoft.com/download/2/c/1/2c16c7e0-96c1-40f5-81fc-3e4bf7b65496/microsoft_asl_compiler-v4-0-0.msi
 #   DDK3790xASL -win32-  Requires:
 # Microsoft Windows Server 2003 Driver Development 
Kit (Microsoft WINDDK) version 3790.1830
 #Optional:
@@ -530,6 +560,13 @@ DEFINE SOURCERY_CYGWIN_TOOLS = /cygdrive/c/Program 
Files/CodeSourcery/Sourcery G
 # Required to build platforms or ACPI tables:
 #   Intel(r) ACPI Compiler (iasl.exe) from
 #   https://acpica.org/downloads
+#   VS2015x86   -win64-  Requires:
+# Microsoft Visual Studio 20

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Here we add the memory space for the memory nodes except the lowest one
>> in FDT. So these spaces will show up in the UEFI memory map.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Shannon Zhao 
>> ---
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 ++
>>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> index 74f80d1..2c8d785 100644
>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>> @@ -27,6 +27,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>  
>>Hob = GetFirstGuidHob(&gFdtHobGuid);
>>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>break;
>>  }
>>  
>> +//
>> +// Check for memory node and add the memory spaces expect the lowest one
>> +//
> 
> (1) Based on the DTB node, which looks like:
> 
> memory {
> reg = <0x0 0x4000 0x0 0x800>;
> device_type = "memory";
> };
> 
> I agree that checking it here, before we look at "compatible", is a good
> thing.
> 
> However, can you please factor this out into a separate function? (Just
> within this file, as a STATIC function.)
> 
> I propose that the function return a BOOLEAN:
> - TRUE if the node was a match (either successfully or unsuccessfully);
>   in which case you can "continue;" with the next iteration directly
> - FALSE, if there is no match (i.e., the current iteration must
>   proceed, in order to look for different device types)
> 

Sure.

>> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>> +  //
>> +  // Get the 'reg' property of this node. For now, we will assume
>> +  // two 8 byte quantities for base and size, respectively.
>> +  //
>> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>> +
>> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>> +
>> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>> +   CurBase, CurSize,
>> +   EFI_MEMORY_WB | EFI_MEMORY_RUNTIME);
> 
> (1) The edk2 coding style requires a space between the function name (or
> function pointer) and the opening paren.
> 
> (2) Regarding the indentation of function arguments, they are aligned
> with the first or (more usually:) second character of the function or
> *member* name. In this case, they should be aligned with the second "d"
> in "AddMemorySpace".
> 
Thanks for explain the edk2 coding style. Sorry, I'm not familiar with this.

> (3) The last argument is a bitmask of capabilities. As far as I
> remember, all system memory regions I've seen dumped by Linux from the
> UEFI memmap at startup had all of: UC | WC | WT | WB.
> 
> I agree that RUNTIME and WB should be listed here, but do you have any
> particular reason for not allowing UC|WC|WT?
> 
> (4) Related question -- when you boot the guest kernel with this patch
> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
> of the additional NUMA nodes look identically to the "base" memory? (I'm
> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
> calls.)
> 
> Can you paste a sample output from "efi=debug"?
> 

Processing EFI memory map:
  0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
  |  |  |  |UC]
  0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
  |  |  |  |UC]
  0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
  |WB|WT|WC|UC]
  0x4001-0x4007 [Conventional Memory|   |  |  |  |  |  |
  |WB|WT|WC|UC]

[cut some information here]

  0x6000-0xbfff [Conventional Memory|RUN|  |  |  |  |  |
  |WB|  |  |  ]

Looks like the region is showed with the attributes we set. While
comparing with other Conventional Memory, it doesn't have RUNTIME. So
drop it?

>> +  if (EFI_ERROR(Status)) {
>> +ASSERT_EFI_ERROR(Status);
>> +  }
> 
> (5) "ASSERT_EFI_ERROR (Status)" would suffice.
> 
> However, I think I'd prefer if we just logged an e

Re: [edk2] [PATCH v2 0/4] ArmVirtPkg: add support for UEFI secure boot to 32-bit ARM

[Ard Biesheuvel]

On 27 November 2015 at 16:21, Ard Biesheuvel  wrote:
> Since UEFI on 32-bit ARM does not allow floating point arithmetic in hardware,
> running OpenSslLib unmodified requires a softfloat library.
>
> This series factors out the minimally required bits of StdLib/LibC/Softfloat,
> and copies them into a new library ArmPkg/Library/ArmSoftFloatLib. This 
> library
> dependency is wired into OpenSslLib, and its resolution added to 
> ArmVirt.dsc.inc
>
> Changes since v1:
> - added patch #2, which adds RVCT support to ArmSoftFloatLib
> - remove --fpu=vfpv3 switch from RVCT command line in patch #4
> - reordered patches
> - added RBs from Leif and Laszlo
>
> Ard Biesheuvel (4):
>   ArmPkg: factor out softfloat support from StdLib/LibC/SoftFloat
>   ArmPkg/ArmSoftFloatLib: add support for RVCT
>   ArmVirtPkg: add secure boot support to 32-bit ARM targets
>   CryptoPkg: add softfloat dependency for ARM
>

Thanks everyone

Committed as SVN r19030 .. r19033

-- 
Ard.


>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c  
>  |   34 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmplt.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpun.c  
>  |   41 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpeq.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpge.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpgt.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmple.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmplt.c  
>  |   36 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpun.c  
>  |   41 +
>  ArmPkg/Library/ArmSoftFloatLib/Arm/softfloat.h   
>  |  345 +++
>  ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf   
>  |   49 +
>  {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/arm-gcc.h 
>  |0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat-macros |0
>  ArmPkg/Library/ArmSoftFloatLib/bits32/softfloat.c
>  | 2354 
>  {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/milieu.h  
>  |0
>  {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/softfloat-for-gcc.h |0
>  ArmPkg/Library/ArmSoftFloatLib/softfloat-specialize  
>  |  525 +
>  ArmVirtPkg/ArmVirt.dsc.inc   
>  |3 +
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf  
>  |5 +-
>  22 files changed, 3720 insertions(+), 1 deletion(-)
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpeq.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpge.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpgt.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmple.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmplt.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_dcmpun.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpeq.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpge.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpgt.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmple.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmplt.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/__aeabi_fcmpun.c
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/Arm/softfloat.h
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
>  copy {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/arm-gcc.h (100%)
>  copy {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/bits32/softfloat-macros (100%)
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/bits32/softfloat.c
>  copy {StdLib/Include/Arm => ArmPkg/Library/ArmSoftFloatLib}/milieu.h (100%)
>  copy {StdLib/LibC/Softfloat => 
> ArmPkg/Library/ArmSoftFloatLib}/softfloat-for-gcc.h (100%)
>  create mode 100644 ArmPkg/Library/ArmSoftFloatLib/softfloat-specialize
>
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/29/15 07:31, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Here we add the memory space for the memory nodes except the lowest one
> in FDT. So these spaces will show up in the UEFI memory map.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Shannon Zhao 
> ---
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c   | 34 ++
>  ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf |  4 
>  2 files changed, 38 insertions(+)
> 
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> index 74f80d1..2c8d785 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>UINT64 FwCfgDataSize;
>UINT64 FwCfgDmaAddress;
>UINT64 FwCfgDmaSize;
> +  UINT64 CurBase;
> +  UINT64 CurSize;
>  
>Hob = GetFirstGuidHob(&gFdtHobGuid);
>if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>break;
>  }
>  
> +//
> +// Check for memory node and add the memory spaces expect the lowest one
> +//

(1) Based on the DTB node, which looks like:

memory {
reg = <0x0 0x4000 0x0 0x800>;
device_type = "memory";
};

I agree that checking it here, before we look at "compatible", is a good
thing.

However, can you please factor this out into a separate function? (Just
within this file, as a STATIC function.)

I propose that the function return a BOOLEAN:
- TRUE if the node was a match (either successfully or unsuccessfully);
  in which case you can "continue;" with the next iteration directly
- FALSE, if there is no match (i.e., the current iteration must
  proceed, in order to look for different device types)

> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +  //
> +  // Get the 'reg' property of this node. For now, we will assume
> +  // two 8 byte quantities for base and size, respectively.
> +  //
> +  RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +  if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
> +
> +if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
> +  Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
> +   CurBase, CurSize,
> +   EFI_MEMORY_WB | EFI_MEMORY_RUNTIME);

(1) The edk2 coding style requires a space between the function name (or
function pointer) and the opening paren.

(2) Regarding the indentation of function arguments, they are aligned
with the first or (more usually:) second character of the function or
*member* name. In this case, they should be aligned with the second "d"
in "AddMemorySpace".

(3) The last argument is a bitmask of capabilities. As far as I
remember, all system memory regions I've seen dumped by Linux from the
UEFI memmap at startup had all of: UC | WC | WT | WB.

I agree that RUNTIME and WB should be listed here, but do you have any
particular reason for not allowing UC|WC|WT?

(4) Related question -- when you boot the guest kernel with this patch
in the firmware, and pass "efi=debug" to Linux, do the memory attributes
of the additional NUMA nodes look identically to the "base" memory? (I'm
asking this to see if we need additional gDS->SetMemorySpaceAttributes()
calls.)

Can you paste a sample output from "efi=debug"?

> +  if (EFI_ERROR(Status)) {
> +ASSERT_EFI_ERROR(Status);
> +  }

(5) "ASSERT_EFI_ERROR (Status)" would suffice.

However, I think I'd prefer if we just logged an error here (with
EFI_D_ERROR level), and continued (i.e., returned TRUE).

> +   DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
> + __FUNCTION__, CurBase, CurBase + CurSize - 1));
> +}

(6) This isn't indented correctly (not just the arguments).

> +  } else {
> +DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
> +   __FUNCTION__));
> +  }

(7) I think this error log branch is not necessary here. The same will
have been logged in the code that is modified by patch #1.

Thanks
Laszlo

> +}
> +
>  Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
>  if (Type == NULL) {
>continue;
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> index ee2503a..0e6927b 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -43,12 +43,16