Re: [edk2] [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Gao, Liming]

Ard and Laszlo:
  Thanks for your quick fix. For my patch, I think it only impacts VS tool 
chain. So, I don't verify GCC tool chain. Sorry for it.  

Reviewed-by: Liming Gao 

Thanks
Liming
>-Original Message-
>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>Sent: Thursday, July 06, 2017 2:34 AM
>To: edk2-devel@lists.01.org; ler...@redhat.com
>Cc: leif.lindh...@linaro.org; Gao, Liming ; Zhu,
>Yonghong ; Ard Biesheuvel
>
>Subject: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory
>entry size
>
>Currently, the PE/COFF conversion routines in GenFw add a so-called
>NB10 CodeView debug record to the image, and update the associated
>directory entry in the PE/COFF optional header to contain its relative
>virtual address (RVA) and size.
>
>However, there are two levels of indirection at work here: the actual
>NB10 CodeView record (which is simply a magic number and some unused
>data fields followed by the NUL terminated filename) is emitted
>separately, and a separate descriptor is emitted that identifies the
>NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and
>records
>its size. The directory entry in the PE/COFF optional header should
>refer to this intermediate descriptor's address and size only, but
>the WriteDebug## () routines in GenFw erroneously record the size of
>both the descriptor and the NB10 CodeView record.
>
>This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
>GenFw to clear unused debug entry generated by VS tool chain",
>2017-06-19), and GenFw now crashes when it attempts to iterate over
>what it thinks are multiple intermediate descriptors for different
>kinds of debug data embedded in the image.
>
>The error is understandable, given that both are carved out of the
>same file space allocation, but this is really an implementation detail
>of GenFw, and is not required. (Note that the intermediate descriptor
>does not require a RVA and so it does not even need to be inside a
>section)
>
>So omit the size of the NB10 CodeView record from the size recorded
>in the optional header.
>
>Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel 
>Co-debugged-or-whatever-by: Laszlo Ersek 
>---
> BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
> BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c
>b/BaseTools/Source/C/GenFw/Elf32Convert.c
>index f7b084dc9b84..14fe4a285857 100644
>--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
>@@ -1142,7 +1142,7 @@ WriteDebug32 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = 
>>Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>b/BaseTools/Source/C/GenFw/Elf64Convert.c
>index 7eed7b92d30f..c39bdff063ab 100644
>--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>@@ -1095,7 +1095,7 @@ WriteDebug64 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = 
>>Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DE
>BUG];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>--
>2.9.3

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

Re: [edk2] [PATCH v2] UefiCpuPkg: ApicLib

[Ni, Ruiyu]

Leo,
Could you please separate the clean-up code into a separate patch?

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Fan, Jeff
> Sent: Thursday, July 6, 2017 9:28 AM
> To: Leo Duran ; edk2-devel@lists.01.org
> Cc: Justen, Jordan L ; Dong, Eric
> ; Gao, Liming 
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg: ApicLib
> 
> Leo,
> 
> How AMD public spec dos define the manner of sending startup IPI to Aps?
> Which chapter is it defined in AMD public spec?
> 
> Does AMD public spec indicate the second startup IPI is not required?
> 
> Thanks!
> Jeff
> 
> -Original Message-
> From: Leo Duran [mailto:leo.du...@amd.com]
> Sent: Sunday, July 02, 2017 6:45 AM
> To: edk2-devel@lists.01.org
> Cc: Leo Duran; Justen, Jordan L; Fan, Jeff; Gao, Liming; Brijesh Singh
> Subject: [PATCH v2] UefiCpuPkg: ApicLib
> 
> 1) SendInitSipiSipi ()
> Skip repeating SendIpi () on AMD processor.
> 
> 2) SendInitSipiSipiAllExcludingSelf ()
> Skip repeating SendIpi () on AMD processor.
> 
> 3) GetProcessorLocationByApicId ()
> Adjust InitialApicId to properly concatenate Package on AMD processor.
> Clean-ups on C Coding standards.
> 
> Cc: Jordan Justen 
> Cc: Jeff Fan 
> Cc: Liming Gao 
> Cc: Brijesh Singh 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran 
> ---
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 52 +
> -
>  .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c| 50 +---
> -
>  2 files changed, 63 insertions(+), 39 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index 2091e5e..a6e4e2e 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -338,7 +338,7 @@ GetInitialApicId (
>AsmCpuid (CPUID_SIGNATURE, , NULL, NULL, NULL);
> 
>//
> -  // If CPUID Leaf B is supported,
> +  // If CPUID Leaf B is supported,
>// And CPUID.0BH:EBX[15:0] reports a non-zero value,
>// Then the initial 32-bit APIC ID = CPUID.0BH:EDX
>// Else the initial 8-bit APIC ID = CPUID.1:EBX[31:24] @@ -554,8 +554,10 @@
> SendInitSipiSipi (
>IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>IcrLow.Bits.Level = 1;
>SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD ()) {
> +MicroSecondDelay (200);
> +SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
> 
>  /**
> @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
>IcrLow.Bits.Level = 1;
>IcrLow.Bits.DestinationShorthand =
> LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD ()) {
> +MicroSecondDelay (200);
> +SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
> 
>  /**
> @@ -1013,13 +1017,14 @@ GetProcessorLocationByApicId (
>UINT32  MaxCoresPerNode;
>UINT32  CorePerNodeMask;
>UINT32  ApicIdShift;
> +  UINT32  ApicIdMask;
>UINTN   ThreadBits;
>UINTN   CoreBits;
> 
>//
>// Check if the processor is capable of supporting more than one logical
> processor.
>//
> -  AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL,
> );
> +  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL,
> + );
>if (VersionInfoEdx.Bits.HTT == 0) {
>  if (Thread != NULL) {
>*Thread = 0;
> @@ -1042,8 +1047,8 @@ GetProcessorLocationByApicId (
>//
>// Get max index of CPUID
>//
> -  AsmCpuid(CPUID_SIGNATURE, , NULL, NULL,
> NULL);
> -  AsmCpuid(CPUID_EXTENDED_FUNCTION, ,
> NULL, NULL, NULL);
> +  AsmCpuid (CPUID_SIGNATURE, , NULL, NULL,
> NULL);
> + AsmCpuid (CPUID_EXTENDED_FUNCTION, ,
> NULL, NULL,
> + NULL);
> 
>//
>// If the extended topology enumeration leaf is available, it @@ -1051,7
> +1056,7 @@ GetProcessorLocationByApicId (
>//
>TopologyLeafSupported = FALSE;
>if (MaxStandardCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> -AsmCpuidEx(
> +AsmCpuidEx (
>CPUID_EXTENDED_TOPOLOGY,
>0,
>,
> @@ -1072,7 +1077,7 @@ GetProcessorLocationByApicId (
>// the SMT sub-field of x2APIC ID.
>//
>LevelType = ExtendedTopologyEcx.Bits.LevelType;
> -  ASSERT(LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> +  ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
>ThreadBits = ExtendedTopologyEax.Bits.ApicIdShift;
> 
>//
> @@ -1081,7 +1086,7 @@ 

Re: [edk2] [Patch 0/4] Update code to follow spec definition.

[Fan, Jeff]

Reviewed-by: Jeff Fan 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Eric Dong
Sent: Thursday, July 06, 2017 9:30 AM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch 0/4] Update code to follow spec definition.

Update return status code for SwitchBSP function to follow spec.

Eric Dong (4):
  MdePkg MpServices: Update return status to follow spec.
  UefiCpuPkg CpuDxe: Update return status to follow spec.
  UefiCpuPkg CpuMpPei: Update return status to follow spec.
  UefiCpuPkg MpInitLib: Update return status to follow spec.

 MdePkg/Include/Ppi/MpServices.h  | 4 ++--
 MdePkg/Include/Protocol/MpService.h  | 4 ++--
 UefiCpuPkg/CpuDxe/CpuMp.c| 2 +-
 UefiCpuPkg/CpuDxe/CpuMp.h| 2 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 2 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   | 2 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.7.0.windows.1

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

[edk2] [Patch 1/4] MdePkg MpServices: Update return status to follow spec.

[Eric Dong]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong 
Cc: Jeff Fan 
---
 MdePkg/Include/Ppi/MpServices.h | 4 ++--
 MdePkg/Include/Protocol/MpService.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Include/Ppi/MpServices.h b/MdePkg/Include/Ppi/MpServices.h
index 2c21d64..37d6b8e 100644
--- a/MdePkg/Include/Ppi/MpServices.h
+++ b/MdePkg/Include/Ppi/MpServices.h
@@ -3,7 +3,7 @@
   This PPI is installed by some platform or chipset-specific PEIM that 
abstracts
   handling multiprocessor support.
 
-  Copyright (c) 2015, Intel Corporation. All rights reserved.
+  Copyright (c) 2015 - 2017, 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
@@ -187,7 +187,7 @@ EFI_STATUS
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to this
   service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or 
a disabled
diff --git a/MdePkg/Include/Protocol/MpService.h 
b/MdePkg/Include/Protocol/MpService.h
index 0dbd150..b76ecd0 100644
--- a/MdePkg/Include/Protocol/MpService.h
+++ b/MdePkg/Include/Protocol/MpService.h
@@ -27,7 +27,7 @@
   APs to help test system memory in parallel with other device initialization.
   Diagnostics applications may also use this protocol for multi-processor.
 
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2017, 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 that accompanies this distribution.
 The full text of the license may be found at
@@ -491,7 +491,7 @@ EFI_STATUS
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to
   this service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or
-- 
2.7.0.windows.1

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

[edk2] [Patch 3/4] UefiCpuPkg CpuMpPei: Update return status to follow spec.

[Eric Dong]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong 
Cc: Jeff Fan 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 2 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index eaf99c7..3b72f44 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -302,7 +302,7 @@ PeiStartupThisAP (
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to this
   service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or 
a disabled
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index 0836593..aae5665 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -271,7 +271,7 @@ PeiStartupThisAP (
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to this
   service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or 
a disabled
-- 
2.7.0.windows.1

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

[edk2] [Patch 0/4] Update code to follow spec definition.

[Eric Dong]

Update return status code for SwitchBSP function to follow spec.

Eric Dong (4):
  MdePkg MpServices: Update return status to follow spec.
  UefiCpuPkg CpuDxe: Update return status to follow spec.
  UefiCpuPkg CpuMpPei: Update return status to follow spec.
  UefiCpuPkg MpInitLib: Update return status to follow spec.

 MdePkg/Include/Ppi/MpServices.h  | 4 ++--
 MdePkg/Include/Protocol/MpService.h  | 4 ++--
 UefiCpuPkg/CpuDxe/CpuMp.c| 2 +-
 UefiCpuPkg/CpuDxe/CpuMp.h| 2 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 2 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   | 2 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.7.0.windows.1

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

[edk2] [Patch 2/4] UefiCpuPkg CpuDxe: Update return status to follow spec.

[Eric Dong]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong 
Cc: Jeff Fan 
---
 UefiCpuPkg/CpuDxe/CpuMp.c | 2 +-
 UefiCpuPkg/CpuDxe/CpuMp.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index 4456946..372c1e3 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -412,7 +412,7 @@ StartupThisAP (
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to
   this service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
index 43ec3bd..d530149 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.h
+++ b/UefiCpuPkg/CpuDxe/CpuMp.h
@@ -375,7 +375,7 @@ StartupThisAP (
   @retval EFI_UNSUPPORTED Switching the BSP cannot be completed prior 
to
   this service returning.
   @retval EFI_UNSUPPORTED Switching the BSP is not supported.
-  @retval EFI_SUCCESS The calling processor is an AP.
+  @retval EFI_DEVICE_ERRORThe calling processor is an AP.
   @retval EFI_NOT_FOUND   The processor with the handle specified by
   ProcessorNumber does not exist.
   @retval EFI_INVALID_PARAMETER   ProcessorNumber specifies the current BSP or
-- 
2.7.0.windows.1

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

[edk2] [Patch 4/4] UefiCpuPkg MpInitLib: Update return status to follow spec.

[Eric Dong]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong 
Cc: Jeff Fan 
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index df19b43..a3eea29 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1584,7 +1584,7 @@ SwitchBSPWorker (
   //
   MpInitLibWhoAmI ();
   if (CallerNumber != CpuMpData->BspNumber) {
-return EFI_SUCCESS;
+return EFI_DEVICE_ERROR;
   }
 
   if (ProcessorNumber >= CpuMpData->CpuCount) {
-- 
2.7.0.windows.1

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

Re: [edk2] [PATCH v2] UefiCpuPkg: ApicLib

[Fan, Jeff]

Leo,

How AMD public spec dos define the manner of sending startup IPI to Aps? Which 
chapter is it defined in AMD public spec?

Does AMD public spec indicate the second startup IPI is not required?

Thanks!
Jeff

-Original Message-
From: Leo Duran [mailto:leo.du...@amd.com] 
Sent: Sunday, July 02, 2017 6:45 AM
To: edk2-devel@lists.01.org
Cc: Leo Duran; Justen, Jordan L; Fan, Jeff; Gao, Liming; Brijesh Singh
Subject: [PATCH v2] UefiCpuPkg: ApicLib

1) SendInitSipiSipi ()
Skip repeating SendIpi () on AMD processor.

2) SendInitSipiSipiAllExcludingSelf ()
Skip repeating SendIpi () on AMD processor.

3) GetProcessorLocationByApicId ()
Adjust InitialApicId to properly concatenate Package on AMD processor.
Clean-ups on C Coding standards.

Cc: Jordan Justen 
Cc: Jeff Fan 
Cc: Liming Gao 
Cc: Brijesh Singh 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leo Duran 
---
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 52 +-
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c| 50 +
 2 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c 
b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index 2091e5e..a6e4e2e 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -338,7 +338,7 @@ GetInitialApicId (
   AsmCpuid (CPUID_SIGNATURE, , NULL, NULL, NULL);
 
   //
-  // If CPUID Leaf B is supported, 
+  // If CPUID Leaf B is supported,
   // And CPUID.0BH:EBX[15:0] reports a non-zero value,
   // Then the initial 32-bit APIC ID = CPUID.0BH:EDX
   // Else the initial 8-bit APIC ID = CPUID.1:EBX[31:24] @@ -554,8 +554,10 @@ 
SendInitSipiSipi (
   IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
   IcrLow.Bits.Level = 1;
   SendIpi (IcrLow.Uint32, ApicId);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, ApicId);
+  if (!StandardSignatureIsAuthenticAMD ()) {
+MicroSecondDelay (200);
+SendIpi (IcrLow.Uint32, ApicId);
+  }
 }
 
 /**
@@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
   IcrLow.Bits.Level = 1;
   IcrLow.Bits.DestinationShorthand = 
LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
   SendIpi (IcrLow.Uint32, 0);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, 0);
+  if (!StandardSignatureIsAuthenticAMD ()) {
+MicroSecondDelay (200);
+SendIpi (IcrLow.Uint32, 0);
+  }
 }
 
 /**
@@ -1013,13 +1017,14 @@ GetProcessorLocationByApicId (
   UINT32  MaxCoresPerNode;
   UINT32  CorePerNodeMask;
   UINT32  ApicIdShift;
+  UINT32  ApicIdMask;
   UINTN   ThreadBits;
   UINTN   CoreBits;
 
   //
   // Check if the processor is capable of supporting more than one logical 
processor.
   //
-  AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL, );
+  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, 
+ );
   if (VersionInfoEdx.Bits.HTT == 0) {
 if (Thread != NULL) {
   *Thread = 0;
@@ -1042,8 +1047,8 @@ GetProcessorLocationByApicId (
   //
   // Get max index of CPUID
   //
-  AsmCpuid(CPUID_SIGNATURE, , NULL, NULL, NULL);
-  AsmCpuid(CPUID_EXTENDED_FUNCTION, , NULL, NULL, NULL);
+  AsmCpuid (CPUID_SIGNATURE, , NULL, NULL, NULL);  
+ AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL, 
+ NULL);
 
   //
   // If the extended topology enumeration leaf is available, it @@ -1051,7 
+1056,7 @@ GetProcessorLocationByApicId (
   //
   TopologyLeafSupported = FALSE;
   if (MaxStandardCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
-AsmCpuidEx(
+AsmCpuidEx (
   CPUID_EXTENDED_TOPOLOGY,
   0,
   ,
@@ -1072,7 +1077,7 @@ GetProcessorLocationByApicId (
   // the SMT sub-field of x2APIC ID.
   //
   LevelType = ExtendedTopologyEcx.Bits.LevelType;
-  ASSERT(LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
+  ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
   ThreadBits = ExtendedTopologyEax.Bits.ApicIdShift;
 
   //
@@ -1081,7 +1086,7 @@ GetProcessorLocationByApicId (
   //
   SubIndex = 1;
   do {
-AsmCpuidEx(
+AsmCpuidEx (
   CPUID_EXTENDED_TOPOLOGY,
   SubIndex,
   ,
@@ -1103,7 +1108,7 @@ GetProcessorLocationByApicId (
 //
 // Get logical processor count
 //
-AsmCpuid(CPUID_VERSION_INFO, NULL, , NULL, NULL);
+AsmCpuid (CPUID_VERSION_INFO, NULL, , NULL, 
+ NULL);
 MaxLogicProcessorsPerPackage = 
VersionInfoEbx.Bits.MaximumAddressableIdsForLogicalProcessors;
 
 //
@@ -1114,11 +1119,11 @@ GetProcessorLocationByApicId (
 //
 // Check for topology extensions on AMD processor
 //
-if (StandardSignatureIsAuthenticAMD()) {
+if (StandardSignatureIsAuthenticAMD ()) {
   if 

Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD)

[Laszlo Ersek]

On 07/06/17 00:31, Brijesh Singh wrote:
> Hi Jordan and Laszlo,
> 
> Ping.
> 
> It has been a while, Do you have any further feedbacks on this series ?
> If you want then I can rebase the patches before you commit into
> upstream repos.

>From my side, refreshing the v7 series (posted on 2017-Jun-22) against
current master would be welcome, just to sort out any context
differences that may have crept in meanwhile.

Other than that, I'm ready to merge v7 (and have been ready since my v7
review, posted on 2017-Jun-23).

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

Re: [edk2] [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD)

[Brijesh Singh]

Hi Jordan and Laszlo,

Ping.

It has been a while, Do you have any further feedbacks on this series ?
If you want then I can rebase the patches before you commit into upstream repos.

-Brijesh

On 05/26/2017 09:43 AM, Brijesh Singh wrote:

The patch series provides support for AMD's new Secure Encrypted
Virtualization (SEV) feature.

SEV is an extension to the AMD-V architecture which supports running
multiple VMs under the control of a hypervisor. The SEV feature allows
the memory contents of a virtual machine (VM) to be transparently encrypted
with a key unique to the guest VM. The memory controller contains a
high performance encryption engine which can be programmed with multiple
keys for use by a different VMs in the system. The programming and
management of these keys is handled by the AMD Secure Processor firmware
which exposes a commands for these tasks.

SEV guest VMs have the concept of private and shared memory.  Private memory is
encrypted with the guest-specific key, while shared memory may be encrypted
with hypervisor key.  Certain types of memory (namely instruction pages and
guest page tables) are always treated as private memory by the hardware.
For data memory, SEV guest VMs can choose which pages they would like to be
private. The choice is done using the standard CPU page tables using the C-bit,
and is fully controlled by the guest. Due to security reasons all the DMA
operations inside the  guest must be performed on shared pages (C-bit clear).
Note that since C-bit is only controllable by the guest OS when it is operating
in 64-bit or 32-bit PAE mode, in all other modes the SEV hardware forces the
C-bit to a 1.

The following links provide additional details:

AMD Memory Encryption whitepaper:
http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf

AMD64 Architecture Programmer's Manual:
 http://support.amd.com/TechDocs/24593.pdf
 SME is section 7.10
 SEV is section 15.34

Secure Encrypted Virutualization Key Management:
http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf

KVM Forum Presentation:
http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtualizatoin_Memory_Encryption_Technology.pdf

[1] http://marc.info/?l=linux-mm=148846752931115=2

---

Patch series is based on commit aff463c825a3
  (Vlv2TbltDevicePkg/FvbRuntimeDxe: correct NumOfLba vararg type in 
EraseBlocks())

https://github.com/codomania/edk2/tree/v6

The patch series is tested with OvmfIa32.dsc, OvmfIa32X64.dsc and OvmfX64.dsc.
Since memory encryption bit is not accessiable when processor is in 32-bit mode
hence any DMA access in this mode would cause assert. I have also tested the
suspend and resume path, it seems to be working fine. I still need to work to
finish adding the SEV Dma support in QemuFwCfgS3Lib package (see TODO).

Changes since v5:
  - add placeholder gIoMmuAbsentProtocolGuid
  - add PlatformHasIoMmuLib
  - fix indentation

Changes since v4:
  - decouple IoMmu protocol implementation from AmdSevDxe into a seperate
IoMmuDxe driver. And introduce a placeholder protocol to provide the
dependency support for the dependent modules.
  - update debug messages to use gEfiCallerBaseName where applicable.
  - fix QemuFwCfgSecLib build errors and simplify SEV support
  - update QemuFwCfgDxeLib to assert when failed to locate IOMMU
  - update comments "host buffer" to " host buffer"

Changes since v3:
  - update AmdSevDxe driver to produce IOMMU protocol
  - remove BmDmaLib dependency
  - update QemuFwCfgLib to use IOMMU protocol to allocate SEV DMA buffer

Changes since v2:
  - move memory encryption CPUID and MSR definition into UefiCpuPkg
  - fix the argument order for SUB instruction in ResetVector and add more
comments
  - update PlatformPei to use BaseMemEncryptSevLib
  - break the overlong comment lines to 79 chars
  - variable aligment and other formating fixes
  - split the SEV DMA support patch for QemuFwCfgLib into multiple patches as
recommended by Laszlo
  - add AmdSevDxe driver which runs very early in DXE phase and clear the C-bit
from MMIO memory region
  - drop 'QemuVideoDxe: Clear C-bit from framebuffer' patch since AmdSevDxe
driver takes care of clearing the C-bit from MMIO region
  - Verified that Qemu PFLASH works fine with SEV guest, Found a KVM driver 
issue
which was causing #PF when PFLASH was enabled. I have submitted patch to
fix it in upstream http://marc.info/?l=kvm=149304930814202=2

Changes since v1:
  - bug fixes in OvmfPkg/ResetVector (pointed by Tom Lendacky)
  - add SEV CPUID and MSR register definition in standard include file
  - remove the MemEncryptLib dependency from PlatformPei. Move 
AmdSevInitialize()
implementation in local file inside the PlatformPei package
  - rename MemCryptSevLib to MemEncryptSevLib and add functions to set or
clear memory encryption attribute on memory region
  - integerate SEV support in BmDmaLib
  - split 

Re: [edk2] [PATCH v2 0/8] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

On 07/04/17 18:56, Laszlo Ersek wrote:
> This is version 2 of the series posted previously at
> .
> 
> Version 2 is a rewrite from scratch based on Jordan's feedback for v1
> and the subsequent discussion.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: extended_tseg_bz1447027_v2
> 
> Cc: Jordan Justen 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (8):
>   OvmfPkg: widen PcdQ35TsegMbytes to UINT16
>   OvmfPkg/PlatformPei: prepare for PcdQ35TsegMbytes becoming dynamic
>   OvmfPkg/SmmAccess: prepare for PcdQ35TsegMbytes becoming dynamic
>   OvmfPkg: make PcdQ35TsegMbytes dynamic
>   OvmfPkg/IndustryStandard/Q35MchIch9.h: add extended TSEG size macros
>   OvmfPkg/SmmAccess: support extended TSEG size
>   OvmfPkg/PlatformPei: honor extended TSEG in PcdQ35TsegMbytes if
> available
>   OvmfPkg: mention the extended TSEG near the PcdQ35TsegMbytes
> declaration
> 
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  4 ++
>  OvmfPkg/OvmfPkg.dec   | 15 +++--
>  OvmfPkg/OvmfPkgIa32.dsc   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc|  1 +
>  OvmfPkg/OvmfPkgX64.dsc|  1 +
>  OvmfPkg/PlatformPei/MemDetect.c   | 67 +++-
>  OvmfPkg/PlatformPei/Platform.c| 14 ++--
>  OvmfPkg/PlatformPei/Platform.h|  7 ++
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.c |  1 +
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf   |  3 +
>  OvmfPkg/SmmAccess/SmmAccessPei.c  | 10 +--
>  OvmfPkg/SmmAccess/SmmAccessPei.inf|  4 +-
>  OvmfPkg/SmmAccess/SmramInternal.c | 25 +++-
>  OvmfPkg/SmmAccess/SmramInternal.h | 13 
>  14 files changed, 142 insertions(+), 24 deletions(-)
> 

Pushed: 253d81c71f67..d04b72c67097.

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

Re: [edk2] [PATCH v2 0/2] OvmfPkg: refresh -D E1000_ENABLE (Intel proprietary driver for e1000)

[Laszlo Ersek]

On 07/04/17 12:00, Laszlo Ersek wrote:
> Version 2 of the series previously posted at
> .
> 
> In this version, LMFA references in the commit message of patch #1 have
> been replaced with Liming's explanation from the mailing list. Patch #2
> has been marked with Jiaxin's feedback tags. No code changes in either
> patch.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: e1000_refresh_v2
> 
> Cc: Ard Biesheuvel 
> Cc: Jiaxin Wu 
> Cc: Jordan Justen 
> Cc: Liming Gao 
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (2):
>   OvmfPkg: disable build-time relocation for DXEFV modules
>   OvmfPkg: update -D E1000_ENABLE from Intel PROEFI v.07 to BootUtil
> v.22
> 
>  OvmfPkg/OvmfPkgIa32.fdf|  6 +---
>  OvmfPkg/OvmfPkgIa32X64.fdf |  3 +-
>  OvmfPkg/OvmfPkgX64.fdf |  3 +-
>  OvmfPkg/README | 31 +---
>  4 files changed, 26 insertions(+), 17 deletions(-)
> 

Pushed: 59541d41633c..253d81c71f67.

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

Re: [edk2] [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Laszlo Ersek]

On 07/05/17 20:33, Ard Biesheuvel wrote:
> Currently, the PE/COFF conversion routines in GenFw add a so-called
> NB10 CodeView debug record to the image, and update the associated
> directory entry in the PE/COFF optional header to contain its relative
> virtual address (RVA) and size.
> 
> However, there are two levels of indirection at work here: the actual
> NB10 CodeView record (which is simply a magic number and some unused
> data fields followed by the NUL terminated filename) is emitted
> separately, and a separate descriptor is emitted that identifies the
> NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and records
> its size. The directory entry in the PE/COFF optional header should
> refer to this intermediate descriptor's address and size only, but
> the WriteDebug## () routines in GenFw erroneously record the size of
> both the descriptor and the NB10 CodeView record.
> 
> This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
> GenFw to clear unused debug entry generated by VS tool chain",
> 2017-06-19), and GenFw now crashes when it attempts to iterate over
> what it thinks are multiple intermediate descriptors for different
> kinds of debug data embedded in the image.
> 
> The error is understandable, given that both are carved out of the
> same file space allocation, but this is really an implementation detail
> of GenFw, and is not required. (Note that the intermediate descriptor
> does not require a RVA and so it does not even need to be inside a
> section)
> 
> So omit the size of the NB10 CodeView record from the size recorded
> in the optional header.
> 
> Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html

Please prepend:

Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012162.html

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> Co-debugged-or-whatever-by: Laszlo Ersek 

Haha, great :)

> ---
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
>  BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Laszlo Ersek 
Tested-by: Laszlo Ersek 

Thanks, Ard!
Laszlo

> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c 
> b/BaseTools/Source/C/GenFw/Elf32Convert.c
> index f7b084dc9b84..14fe4a285857 100644
> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
> @@ -1142,7 +1142,7 @@ WriteDebug32 (
>NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>DataDir = 
> >Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>  }
>  
>  STATIC
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c 
> b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index 7eed7b92d30f..c39bdff063ab 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -1095,7 +1095,7 @@ WriteDebug64 (
>NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>DataDir = 
> >Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>  }
>  
>  STATIC
> 

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

[edk2] [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Ard Biesheuvel]

Currently, the PE/COFF conversion routines in GenFw add a so-called
NB10 CodeView debug record to the image, and update the associated
directory entry in the PE/COFF optional header to contain its relative
virtual address (RVA) and size.

However, there are two levels of indirection at work here: the actual
NB10 CodeView record (which is simply a magic number and some unused
data fields followed by the NUL terminated filename) is emitted
separately, and a separate descriptor is emitted that identifies the
NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and records
its size. The directory entry in the PE/COFF optional header should
refer to this intermediate descriptor's address and size only, but
the WriteDebug## () routines in GenFw erroneously record the size of
both the descriptor and the NB10 CodeView record.

This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
GenFw to clear unused debug entry generated by VS tool chain",
2017-06-19), and GenFw now crashes when it attempts to iterate over
what it thinks are multiple intermediate descriptors for different
kinds of debug data embedded in the image.

The error is understandable, given that both are carved out of the
same file space allocation, but this is really an implementation detail
of GenFw, and is not required. (Note that the intermediate descriptor
does not require a RVA and so it does not even need to be inside a
section)

So omit the size of the NB10 CodeView record from the size recorded
in the optional header.

Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Co-debugged-or-whatever-by: Laszlo Ersek 
---
 BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
 BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c 
b/BaseTools/Source/C/GenFw/Elf32Convert.c
index f7b084dc9b84..14fe4a285857 100644
--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
@@ -1142,7 +1142,7 @@ WriteDebug32 (
   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
   DataDir = 
>Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
   DataDir->VirtualAddress = mDebugOffset;
-  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
 }
 
 STATIC
diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c 
b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 7eed7b92d30f..c39bdff063ab 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -1095,7 +1095,7 @@ WriteDebug64 (
   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
   DataDir = 
>Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
   DataDir->VirtualAddress = mDebugOffset;
-  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
 }
 
 STATIC
-- 
2.9.3

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

Re: [edk2] [PATCH v2 0/8] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

On 07/05/17 19:31, Jordan Justen wrote:
> On 2017-07-04 09:56:21, Laszlo Ersek wrote:
>> This is version 2 of the series posted previously at
>> .
>>
>> Version 2 is a rewrite from scratch based on Jordan's feedback for v1
>> and the subsequent discussion.
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: extended_tseg_bz1447027_v2
>>
>> Cc: Jordan Justen 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (8):
>>   OvmfPkg: widen PcdQ35TsegMbytes to UINT16
>>   OvmfPkg/PlatformPei: prepare for PcdQ35TsegMbytes becoming dynamic
>>   OvmfPkg/SmmAccess: prepare for PcdQ35TsegMbytes becoming dynamic
>
> Maybe InitQ35TsegMbytes instead of FetchQ35TsegMbytes.

OK, I'll do this rename before I push.

> I also thought
> a GetQ35TsegMbytes function could just return the value rather than
> adding an externally visible global. Not a big deal.
>
>>   OvmfPkg: make PcdQ35TsegMbytes dynamic
>>   OvmfPkg/IndustryStandard/Q35MchIch9.h: add extended TSEG size macros
>>   OvmfPkg/SmmAccess: support extended TSEG size
>
> Some indentation issues in SmramInternal.c.

Do you mean this hunk:

> diff --git a/OvmfPkg/SmmAccess/SmramInternal.c 
> b/OvmfPkg/SmmAccess/SmramInternal.c
> index ae1e9069aca6..fa0efeda72b0 100644
> --- a/OvmfPkg/SmmAccess/SmramInternal.c
> +++ b/OvmfPkg/SmmAccess/SmramInternal.c
> @@ -196,9 +196,11 @@ SmramAccessGetCapabilities (
>  SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;
>SmramMap[DescIdxMain].CpuStart = SmramMap[DescIdxMain].PhysicalStart;
>SmramMap[DescIdxMain].PhysicalSize =
> -(TsegSizeBits == MCH_ESMRAMC_TSEG_8MB ? SIZE_8MB :
> - TsegSizeBits == MCH_ESMRAMC_TSEG_2MB ? SIZE_2MB :
> - SIZE_1MB) - SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;
> + (TsegSizeBits == MCH_ESMRAMC_TSEG_8MB ? 
> SIZE_8MB :
> +  TsegSizeBits == MCH_ESMRAMC_TSEG_2MB ? 
> SIZE_2MB :
> +  TsegSizeBits == MCH_ESMRAMC_TSEG_1MB ? 
> SIZE_1MB :
> +  mQ35TsegMbytes * SIZE_1MB) -
> + SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;

I'll align the added code with the removed code before I push.

>
>>   OvmfPkg/PlatformPei: honor extended TSEG in PcdQ35TsegMbytes if
>> available
>>   OvmfPkg: mention the extended TSEG near the PcdQ35TsegMbytes
>> declaration
>
> Series Reviewed-by: Jordan Justen 

Thanks!
Laszlo

>
>>
>>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  4 ++
>>  OvmfPkg/OvmfPkg.dec   | 15 +++--
>>  OvmfPkg/OvmfPkgIa32.dsc   |  1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc|  1 +
>>  OvmfPkg/OvmfPkgX64.dsc|  1 +
>>  OvmfPkg/PlatformPei/MemDetect.c   | 67 +++-
>>  OvmfPkg/PlatformPei/Platform.c| 14 ++--
>>  OvmfPkg/PlatformPei/Platform.h|  7 ++
>>  OvmfPkg/SmmAccess/SmmAccess2Dxe.c |  1 +
>>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf   |  3 +
>>  OvmfPkg/SmmAccess/SmmAccessPei.c  | 10 +--
>>  OvmfPkg/SmmAccess/SmmAccessPei.inf|  4 +-
>>  OvmfPkg/SmmAccess/SmramInternal.c | 25 +++-
>>  OvmfPkg/SmmAccess/SmramInternal.h | 13 
>>  14 files changed, 142 insertions(+), 24 deletions(-)
>>
>> --
>> 2.13.1.3.g8be5a757fa67
>>

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

Re: [edk2] [PATCH 1/2] OvmfPkg: disable build-time relocation for DXEFV modules

[Laszlo Ersek]

On 07/05/17 19:48, Jordan Justen wrote:
> On 2017-06-28 20:32:54, Gao, Liming wrote:
>> Laszlo:
>>   LMFA feature doesn't do PE image rebase at build time. Only XIP
>>   module needs to be rebased at build time. LMFA feature will
>>   specify the loaded memory address for each PE image. At build
>>   time, build tool records the memory address into the one field of
>>   PE image. It doesn't rebase PE image. At boot time,
>>   PeiCore/DxeCore/SmmCore will parse PE image, and try to load it at
>>   the preferred memory address. If the preferred memory address is
>>   not available, PE image will be loaded to other memory address.
>>   LMFA feature only supports the source build EFI image, not support
>>   the binary EFI image. This is a debug feature.
>>
>>   For this case, OvmfPkg DXEFV doesn't require to run as XIP. So, it
>>   doesn't require rebase. I agree this change.
>>
> 
> Liming,
> 
> With this flag set to FALSE, do we attempt to rebase the images, but
> just not fail if we can't rebase one of the modules? Or, will this
> disable rebasing of all modules in the FV?
> 
> I don't think we really make use of this debug feature in OVMF today,
> but I was wondering what potential impact it might have. I thought in
> some cases by loading the modules at a fixed address it can help with
> loading symbols for debug, right?
> 
> Laszlo,
> 
> I don't think you need to hold off on the patches for this answer, so
> you can add an Acked-by from me for your v2.

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

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek]

On 07/05/17 19:37, Ard Biesheuvel wrote:
> On 5 July 2017 at 18:33, Laszlo Ersek  wrote:
>> On 07/05/17 18:45, Ard Biesheuvel wrote:
>>> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
 GNU Binutils produce a PE debug directory with one
>>>
>>> This sentence already confuses me. This crash is reproducible on ARM,
>>> but the ARM toolchains are strictly ELF based, and all PE/COFF data
>>> structures are created by GenFw itself, never by binutils. So I don't
>>> see how this could be a binutils bug.
>>
>> Geez, you are totally right. From
>> "BaseTools/Source/C/GenFw/Elf64Convert.c":
>>
>>
>>> STATIC
>>> VOID
>>> WriteDebug64 (
>>>   VOID
>>>   )
>>> {
>>>   UINT32  Len;
>>>   EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
>>>   EFI_IMAGE_DATA_DIRECTORY*DataDir;
>>>   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
>>>   EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
>>>
>>>   Len = strlen(mInImageName) + 1;
>>>
>>>   Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
>>>   Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>>>   Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
>>>   Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>>   Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>>
>>>   Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>>>   Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>>>   strcpy ((char *)(Nb10 + 1), mInImageName);
>>>
>>>
>>>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>>>   DataDir = 
>>> >Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>>>   DataDir->VirtualAddress = mDebugOffset;
>>>   DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>> }
>>
>> The last assignment has the bug. It should be
>>
>>   DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>
> 
> OK, I will take that as an affirmative answer to my question. Are you
> sending a patch?
> 

You send it please, just give me some "Co-debugged-by:" or whatever. :)
Also, please add a ref to the mailing list thread.

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

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek]

On 07/05/17 19:33, Ard Biesheuvel wrote:
> On 5 July 2017 at 18:28, Laszlo Ersek  wrote:
>> On 07/05/17 18:45, Ard Biesheuvel wrote:
>>> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
 GNU Binutils produce a PE debug directory with one
>>>
>>> This sentence already confuses me. This crash is reproducible on ARM,
>>> but the ARM toolchains are strictly ELF based, and all PE/COFF data
>>> structures are created by GenFw itself, never by binutils.
>>
>> According to binutils commit 61e2488cd849:
>>
>>   Add support for generating and inserting build IDs into COFF binaries.
>>
>>   
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=61e2488cd849
>>
>> the write_build_id() function from that commit does produce PE/COFF
>> artifacts.
>>
>>   /* Construct a debug directory entry which points to an immediately 
>> following CodeView record.  */
>>
>>   /* Record the location of the debug directory in the data directory.  */
>>
>> I can't exactly say where the bug is (it may have been added later --
>> I'm not a binutils developer), and the code I quoted above might not
>> even be related to the symptoms we're seeing at all, but binutils can
>> definitely generate PE stuff.
>>
>> Plus, the mal-sized debug directory in the GenFw-crasher DLL files seems
>> to fall onto a section called ".build-id".
>>
>> OTOH, after reviewing the commands from Gerd's Jenkins log that lead to
>> the GenFw crash on "SecMain.dll", I think you are right... All of these
>> commands use ELF formats, apparently:
>>
>>> "gcc" \
>>>   -g \
>>>   -fshort-wchar \
>>>   -fno-builtin \
>>>   -fno-strict-aliasing \
>>>   -Wall \
>>>   -Werror \
>>>   -Wno-array-bounds \
>>>   -ffunction-sections \
>>>   -fdata-sections \
>>>   -include AutoGen.h \
>>>   -fno-common \
>>>   -DSTRING_ARRAY_NAME=SecMainStrings \
>>>   -m32 \
>>>   -march=i586 \
>>>   -malign-double \
>>>   -fno-stack-protector \
>>>   -D EFI32 \
>>>   -fno-asynchronous-unwind-tables \
>>>   -Wno-address \
>>>   -Os \
>>>   -mno-mmx \
>>>   -mno-sse \
>>>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>>>   -c \
>>>   -o 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./SecMain.obj \
>>>   -IOvmfPkg/Sec/Ia32 \
>>>   -IOvmfPkg/Sec \
>>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>>   -IMdePkg \
>>>   -IMdePkg/Include \
>>>   -IMdePkg/Include/Ia32 \
>>>   -IMdeModulePkg \
>>>   -IMdeModulePkg/Include \
>>>   -IUefiCpuPkg \
>>>   -IUefiCpuPkg/Include \
>>>   -IOvmfPkg \
>>>   -IOvmfPkg/Include \
>>>   OvmfPkg/Sec/SecMain.c
>>>
>>> "gcc" \
>>>   -E \
>>>   -x assembler-with-cpp \
>>>   -include 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.h \
>>>   -IOvmfPkg/Sec/Ia32 \
>>>   -IOvmfPkg/Sec \
>>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>>   -IMdePkg \
>>>   -IMdePkg/Include \
>>>   -IMdePkg/Include/Ia32 \
>>>   -IMdeModulePkg \
>>>   -IMdeModulePkg/Include \
>>>   -IUefiCpuPkg \
>>>   -IUefiCpuPkg/Include \
>>>   -IOvmfPkg \
>>>   -IOvmfPkg/Include \
>>>   OvmfPkg/Sec/Ia32/SecEntry.nasm \
>>>   > 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>>>
>>> Trim \
>>>   --trim-long \
>>>   --source-code \
>>>   -o 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii
>>>  \
>>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>>>
>>> "nasm" \
>>>   -IOvmfPkg/Sec/Ia32/ \
>>>   -f elf32 \
>>>   -o 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.obj
>>>  \
>>>   
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii
>>>
>>> "gcc" \
>>>   -g \
>>>   -fshort-wchar \
>>>   -fno-builtin \
>>>   -fno-strict-aliasing \
>>>   -Wall \
>>>   -Werror \
>>>   -Wno-array-bounds \
>>>   -ffunction-sections \
>>>   -fdata-sections \
>>>   -include AutoGen.h \
>>>   -fno-common \
>>>   -DSTRING_ARRAY_NAME=SecMainStrings \
>>>   -m32 \
>>>   -march=i586 \
>>>   -malign-double \
>>>   -fno-stack-protector \
>>>   -D EFI32 \
>>>   -fno-asynchronous-unwind-tables \
>>>   -Wno-address \
>>>   -Os \
>>>   -mno-mmx \
>>>   -mno-sse \
>>>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>>>   -c \
>>>   -o 
>>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./AutoGen.obj \
>>>   -IOvmfPkg/Sec/Ia32 \
>>>   -IOvmfPkg/Sec \
>>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>>   -IMdePkg \
>>>   -IMdePkg/Include \
>>>   -IMdePkg/Include/Ia32 \
>>>   -IMdeModulePkg \
>>>   -IMdeModulePkg/Include \
>>>   -IUefiCpuPkg \
>>>   -IUefiCpuPkg/Include \
>>>   -IOvmfPkg \
>>>   -IOvmfPkg/Include \
>>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c
>>>
>>> "ar" \
>>>   cr \
>>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/SecMain.lib \
>>>   
>>> @Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/object_files.lst
>>>
>>> "gcc" \
>>>   -o 

Re: [edk2] [PATCH 1/2] OvmfPkg: disable build-time relocation for DXEFV modules

[Jordan Justen]

On 2017-06-28 20:32:54, Gao, Liming wrote:
> Laszlo:
>   LMFA feature doesn't do PE image rebase at build time. Only XIP
>   module needs to be rebased at build time. LMFA feature will
>   specify the loaded memory address for each PE image. At build
>   time, build tool records the memory address into the one field of
>   PE image. It doesn't rebase PE image. At boot time,
>   PeiCore/DxeCore/SmmCore will parse PE image, and try to load it at
>   the preferred memory address. If the preferred memory address is
>   not available, PE image will be loaded to other memory address.
>   LMFA feature only supports the source build EFI image, not support
>   the binary EFI image. This is a debug feature.
> 
>   For this case, OvmfPkg DXEFV doesn't require to run as XIP. So, it
>   doesn't require rebase. I agree this change.
> 

Liming,

With this flag set to FALSE, do we attempt to rebase the images, but
just not fail if we can't rebase one of the modules? Or, will this
disable rebasing of all modules in the FV?

I don't think we really make use of this debug feature in OVMF today,
but I was wondering what potential impact it might have. I thought in
some cases by loading the modules at a fixed address it can help with
loading symbols for debug, right?

Laszlo,

I don't think you need to hold off on the patches for this answer, so
you can add an Acked-by from me for your v2.

-Jordan

>
> >-Original Message-
> >From: Laszlo Ersek [mailto:ler...@redhat.com]
> >Sent: Thursday, June 29, 2017 6:07 AM
> >To: edk2-devel-01 
> >Cc: Ard Biesheuvel ; Justen, Jordan L
> >; Gao, Liming 
> >Subject: [PATCH 1/2] OvmfPkg: disable build-time relocation for DXEFV
> >modules
> >
> >When the GenFv utility from BaseTools composes a firmware volume, it
> >checks whether modules in the firmware volume are subject to build-time
> >relocation. The primary indication for relocation is whether the firmware
> >volume has a nonzero base address, according to the [FD] section(s) in the
> >FDF file that refer to the firmware volume.
> >
> >The idea behind build-time relocation is that XIP (execute in place)
> >modules will not be relocated at boot-time:
> >
> >- Pre-DXE phase modules generally execute in place.
> >
> >  (OVMF is no exception, despite the fact that we have writeable memory
> >  even in SEC: PEI_CORE and PEIMs run in-place from PEIFV, after SEC
> >  decompresses PEIFV and DXEFV from FVMAIN_COMPACT (flash) to RAM.
> >  PEI_CORE and the PEIMs are relocated at boot-time only after PlatformPei
> >  installs the permanent PEI RAM, and the RAM migration occurs.)
> >
> >- Modules dispatched by the DXE Core are generally relocated at boot-time.
> >  However, this is not necessarily so, the LMFA (Load Modules at Fixed
> >  Address) feature apparently allows in-place execution for such modules
> >  as well, deriving the load address from the containing firmware volume's
> >  base address at build time.
> >
> >  (LMFA is controlled by the
> >  gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable
> >fixed
> >  PCD, which we leave disabled in OVMF.)
> >
> >Therefore GenFv relocates even DXE and UEFI driver modules if the
> >containing firmware volume has a nonzero base address.
> >
> >In OVMF, this is the case for both PEIV and DXEFV:
> >
> >> [FD.MEMFD]
> >> BaseAddress   = $(MEMFD_BASE_ADDRESS)
> >> Size  = 0xB0
> >> ErasePolarity = 1
> >> BlockSize = 0x1
> >> NumBlocks = 0xB0
> >> ...
> >> 0x02|0x0E
> >>
> >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgToke
> >nSpaceGuid.PcdOvmfPeiMemFvSize
> >> FV = PEIFV
> >>
> >> 0x10|0xA0
> >>
> >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTok
> >enSpaceGuid.PcdOvmfDxeMemFvSize
> >> FV = DXEFV
> >
> >While the build-time relocation certainly makes sense for PEIFV (see
> >above), the reasons for which we specify DXEFV under [FD.MEMFD] are
> >weaker:
> >
> >- we set the PcdOvmfDxeMemFvBase and PcdOvmfDxeMemFvSize PCDs
> >here,
> >
> >- and we ascertain that DXEFV, when decompressed by SEC from
> >  FVMAIN_COMPACT, will fit into the area allotted here, at build time.
> >
> >In other words, the build-time relocation of the modules in DXEFV is a
> >waste of resources. But, it gets worse:
> >
> >Build-time relocation of an executable is only possible if the on-disk and
> >in-memory layouts are identical, i.e., if the sections of the PE/COFF
> >image adhere to the same alignment on disk and in memory. Put differently,
> >the FileAlignment and SectionAlignment headers must be equal.
> >
> >For boot-time modules that we build as part of edk2, both alignment values
> >are 0x20 bytes. For runtime modules that we build as part of edk2, both
> >alignment values are 0x1000 bytes. This is why the DXEFV relocation,
> >albeit wasteful, is also successful every time.
> >
> >Unfortunately, if we try to include a PE/COFF binary in 

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Ard Biesheuvel]

On 5 July 2017 at 18:33, Laszlo Ersek  wrote:
> On 07/05/17 18:45, Ard Biesheuvel wrote:
>> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
>>> GNU Binutils produce a PE debug directory with one
>>
>> This sentence already confuses me. This crash is reproducible on ARM,
>> but the ARM toolchains are strictly ELF based, and all PE/COFF data
>> structures are created by GenFw itself, never by binutils. So I don't
>> see how this could be a binutils bug.
>
> Geez, you are totally right. From
> "BaseTools/Source/C/GenFw/Elf64Convert.c":
>
>
>> STATIC
>> VOID
>> WriteDebug64 (
>>   VOID
>>   )
>> {
>>   UINT32  Len;
>>   EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
>>   EFI_IMAGE_DATA_DIRECTORY*DataDir;
>>   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
>>   EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
>>
>>   Len = strlen(mInImageName) + 1;
>>
>>   Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
>>   Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>>   Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
>>   Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>   Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>
>>   Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>>   Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>>   strcpy ((char *)(Nb10 + 1), mInImageName);
>>
>>
>>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>>   DataDir = 
>> >Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>>   DataDir->VirtualAddress = mDebugOffset;
>>   DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> }
>
> The last assignment has the bug. It should be
>
>   DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>

OK, I will take that as an affirmative answer to my question. Are you
sending a patch?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek]

On 07/05/17 18:45, Ard Biesheuvel wrote:
> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
>> GNU Binutils produce a PE debug directory with one
>
> This sentence already confuses me. This crash is reproducible on ARM,
> but the ARM toolchains are strictly ELF based, and all PE/COFF data
> structures are created by GenFw itself, never by binutils. So I don't
> see how this could be a binutils bug.

Geez, you are totally right. From
"BaseTools/Source/C/GenFw/Elf64Convert.c":


> STATIC
> VOID
> WriteDebug64 (
>   VOID
>   )
> {
>   UINT32  Len;
>   EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
>   EFI_IMAGE_DATA_DIRECTORY*DataDir;
>   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
>   EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
> 
>   Len = strlen(mInImageName) + 1;
> 
>   Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
>   Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>   Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
>   Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>   Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> 
>   Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>   Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>   strcpy ((char *)(Nb10 + 1), mInImageName);
> 
> 
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>   DataDir = 
> >Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>   DataDir->VirtualAddress = mDebugOffset;
>   DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }

The last assignment has the bug. It should be

  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);

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

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Ard Biesheuvel]

On 5 July 2017 at 18:28, Laszlo Ersek  wrote:
> On 07/05/17 18:45, Ard Biesheuvel wrote:
>> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
>>> GNU Binutils produce a PE debug directory with one
>>
>> This sentence already confuses me. This crash is reproducible on ARM,
>> but the ARM toolchains are strictly ELF based, and all PE/COFF data
>> structures are created by GenFw itself, never by binutils.
>
> According to binutils commit 61e2488cd849:
>
>   Add support for generating and inserting build IDs into COFF binaries.
>
>   
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=61e2488cd849
>
> the write_build_id() function from that commit does produce PE/COFF
> artifacts.
>
>   /* Construct a debug directory entry which points to an immediately 
> following CodeView record.  */
>
>   /* Record the location of the debug directory in the data directory.  */
>
> I can't exactly say where the bug is (it may have been added later --
> I'm not a binutils developer), and the code I quoted above might not
> even be related to the symptoms we're seeing at all, but binutils can
> definitely generate PE stuff.
>
> Plus, the mal-sized debug directory in the GenFw-crasher DLL files seems
> to fall onto a section called ".build-id".
>
> OTOH, after reviewing the commands from Gerd's Jenkins log that lead to
> the GenFw crash on "SecMain.dll", I think you are right... All of these
> commands use ELF formats, apparently:
>
>> "gcc" \
>>   -g \
>>   -fshort-wchar \
>>   -fno-builtin \
>>   -fno-strict-aliasing \
>>   -Wall \
>>   -Werror \
>>   -Wno-array-bounds \
>>   -ffunction-sections \
>>   -fdata-sections \
>>   -include AutoGen.h \
>>   -fno-common \
>>   -DSTRING_ARRAY_NAME=SecMainStrings \
>>   -m32 \
>>   -march=i586 \
>>   -malign-double \
>>   -fno-stack-protector \
>>   -D EFI32 \
>>   -fno-asynchronous-unwind-tables \
>>   -Wno-address \
>>   -Os \
>>   -mno-mmx \
>>   -mno-sse \
>>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>>   -c \
>>   -o 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./SecMain.obj \
>>   -IOvmfPkg/Sec/Ia32 \
>>   -IOvmfPkg/Sec \
>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>   -IMdePkg \
>>   -IMdePkg/Include \
>>   -IMdePkg/Include/Ia32 \
>>   -IMdeModulePkg \
>>   -IMdeModulePkg/Include \
>>   -IUefiCpuPkg \
>>   -IUefiCpuPkg/Include \
>>   -IOvmfPkg \
>>   -IOvmfPkg/Include \
>>   OvmfPkg/Sec/SecMain.c
>>
>> "gcc" \
>>   -E \
>>   -x assembler-with-cpp \
>>   -include 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.h \
>>   -IOvmfPkg/Sec/Ia32 \
>>   -IOvmfPkg/Sec \
>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>   -IMdePkg \
>>   -IMdePkg/Include \
>>   -IMdePkg/Include/Ia32 \
>>   -IMdeModulePkg \
>>   -IMdeModulePkg/Include \
>>   -IUefiCpuPkg \
>>   -IUefiCpuPkg/Include \
>>   -IOvmfPkg \
>>   -IOvmfPkg/Include \
>>   OvmfPkg/Sec/Ia32/SecEntry.nasm \
>>   > 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>>
>> Trim \
>>   --trim-long \
>>   --source-code \
>>   -o 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii 
>> \
>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>>
>> "nasm" \
>>   -IOvmfPkg/Sec/Ia32/ \
>>   -f elf32 \
>>   -o 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.obj 
>> \
>>   
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii
>>
>> "gcc" \
>>   -g \
>>   -fshort-wchar \
>>   -fno-builtin \
>>   -fno-strict-aliasing \
>>   -Wall \
>>   -Werror \
>>   -Wno-array-bounds \
>>   -ffunction-sections \
>>   -fdata-sections \
>>   -include AutoGen.h \
>>   -fno-common \
>>   -DSTRING_ARRAY_NAME=SecMainStrings \
>>   -m32 \
>>   -march=i586 \
>>   -malign-double \
>>   -fno-stack-protector \
>>   -D EFI32 \
>>   -fno-asynchronous-unwind-tables \
>>   -Wno-address \
>>   -Os \
>>   -mno-mmx \
>>   -mno-sse \
>>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>>   -c \
>>   -o 
>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./AutoGen.obj \
>>   -IOvmfPkg/Sec/Ia32 \
>>   -IOvmfPkg/Sec \
>>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>>   -IMdePkg \
>>   -IMdePkg/Include \
>>   -IMdePkg/Include/Ia32 \
>>   -IMdeModulePkg \
>>   -IMdeModulePkg/Include \
>>   -IUefiCpuPkg \
>>   -IUefiCpuPkg/Include \
>>   -IOvmfPkg \
>>   -IOvmfPkg/Include \
>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c
>>
>> "ar" \
>>   cr \
>>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/SecMain.lib \
>>   
>> @Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/object_files.lst
>>
>> "gcc" \
>>   -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll \
>>   -nostdlib \
>>   -Wl,-n,-q,--gc-sections \
>>   -z common-page-size=0x40 \
>>   -Wl,--entry,_ModuleEntryPoint \
>>   -u _ModuleEntryPoint \
>>   
>> 

Re: [edk2] [PATCH v2 0/8] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Jordan Justen]

On 2017-07-04 09:56:21, Laszlo Ersek wrote:
> This is version 2 of the series posted previously at
> .
> 
> Version 2 is a rewrite from scratch based on Jordan's feedback for v1
> and the subsequent discussion.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: extended_tseg_bz1447027_v2
> 
> Cc: Jordan Justen 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (8):
>   OvmfPkg: widen PcdQ35TsegMbytes to UINT16
>   OvmfPkg/PlatformPei: prepare for PcdQ35TsegMbytes becoming dynamic
>   OvmfPkg/SmmAccess: prepare for PcdQ35TsegMbytes becoming dynamic

Maybe InitQ35TsegMbytes instead of FetchQ35TsegMbytes. I also thought
a GetQ35TsegMbytes function could just return the value rather than
adding an externally visible global. Not a big deal.

>   OvmfPkg: make PcdQ35TsegMbytes dynamic
>   OvmfPkg/IndustryStandard/Q35MchIch9.h: add extended TSEG size macros
>   OvmfPkg/SmmAccess: support extended TSEG size

Some indentation issues in SmramInternal.c.

>   OvmfPkg/PlatformPei: honor extended TSEG in PcdQ35TsegMbytes if
> available
>   OvmfPkg: mention the extended TSEG near the PcdQ35TsegMbytes
> declaration

Series Reviewed-by: Jordan Justen 

> 
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  4 ++
>  OvmfPkg/OvmfPkg.dec   | 15 +++--
>  OvmfPkg/OvmfPkgIa32.dsc   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc|  1 +
>  OvmfPkg/OvmfPkgX64.dsc|  1 +
>  OvmfPkg/PlatformPei/MemDetect.c   | 67 +++-
>  OvmfPkg/PlatformPei/Platform.c| 14 ++--
>  OvmfPkg/PlatformPei/Platform.h|  7 ++
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.c |  1 +
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf   |  3 +
>  OvmfPkg/SmmAccess/SmmAccessPei.c  | 10 +--
>  OvmfPkg/SmmAccess/SmmAccessPei.inf|  4 +-
>  OvmfPkg/SmmAccess/SmramInternal.c | 25 +++-
>  OvmfPkg/SmmAccess/SmramInternal.h | 13 
>  14 files changed, 142 insertions(+), 24 deletions(-)
> 
> -- 
> 2.13.1.3.g8be5a757fa67
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek]

On 07/05/17 18:45, Ard Biesheuvel wrote:
> On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
>> GNU Binutils produce a PE debug directory with one
>
> This sentence already confuses me. This crash is reproducible on ARM,
> but the ARM toolchains are strictly ELF based, and all PE/COFF data
> structures are created by GenFw itself, never by binutils.

According to binutils commit 61e2488cd849:

  Add support for generating and inserting build IDs into COFF binaries.

  
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=61e2488cd849

the write_build_id() function from that commit does produce PE/COFF
artifacts.

  /* Construct a debug directory entry which points to an immediately following 
CodeView record.  */

  /* Record the location of the debug directory in the data directory.  */

I can't exactly say where the bug is (it may have been added later --
I'm not a binutils developer), and the code I quoted above might not
even be related to the symptoms we're seeing at all, but binutils can
definitely generate PE stuff.

Plus, the mal-sized debug directory in the GenFw-crasher DLL files seems
to fall onto a section called ".build-id".

OTOH, after reviewing the commands from Gerd's Jenkins log that lead to
the GenFw crash on "SecMain.dll", I think you are right... All of these
commands use ELF formats, apparently:

> "gcc" \
>   -g \
>   -fshort-wchar \
>   -fno-builtin \
>   -fno-strict-aliasing \
>   -Wall \
>   -Werror \
>   -Wno-array-bounds \
>   -ffunction-sections \
>   -fdata-sections \
>   -include AutoGen.h \
>   -fno-common \
>   -DSTRING_ARRAY_NAME=SecMainStrings \
>   -m32 \
>   -march=i586 \
>   -malign-double \
>   -fno-stack-protector \
>   -D EFI32 \
>   -fno-asynchronous-unwind-tables \
>   -Wno-address \
>   -Os \
>   -mno-mmx \
>   -mno-sse \
>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>   -c \
>   -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./SecMain.obj 
> \
>   -IOvmfPkg/Sec/Ia32 \
>   -IOvmfPkg/Sec \
>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>   -IMdePkg \
>   -IMdePkg/Include \
>   -IMdePkg/Include/Ia32 \
>   -IMdeModulePkg \
>   -IMdeModulePkg/Include \
>   -IUefiCpuPkg \
>   -IUefiCpuPkg/Include \
>   -IOvmfPkg \
>   -IOvmfPkg/Include \
>   OvmfPkg/Sec/SecMain.c
>
> "gcc" \
>   -E \
>   -x assembler-with-cpp \
>   -include 
> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.h \
>   -IOvmfPkg/Sec/Ia32 \
>   -IOvmfPkg/Sec \
>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>   -IMdePkg \
>   -IMdePkg/Include \
>   -IMdePkg/Include/Ia32 \
>   -IMdeModulePkg \
>   -IMdeModulePkg/Include \
>   -IUefiCpuPkg \
>   -IUefiCpuPkg/Include \
>   -IOvmfPkg \
>   -IOvmfPkg/Include \
>   OvmfPkg/Sec/Ia32/SecEntry.nasm \
>   > Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>
> Trim \
>   --trim-long \
>   --source-code \
>   -o 
> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii \
>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i
>
> "nasm" \
>   -IOvmfPkg/Sec/Ia32/ \
>   -f elf32 \
>   -o 
> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.obj \
>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii
>
> "gcc" \
>   -g \
>   -fshort-wchar \
>   -fno-builtin \
>   -fno-strict-aliasing \
>   -Wall \
>   -Werror \
>   -Wno-array-bounds \
>   -ffunction-sections \
>   -fdata-sections \
>   -include AutoGen.h \
>   -fno-common \
>   -DSTRING_ARRAY_NAME=SecMainStrings \
>   -m32 \
>   -march=i586 \
>   -malign-double \
>   -fno-stack-protector \
>   -D EFI32 \
>   -fno-asynchronous-unwind-tables \
>   -Wno-address \
>   -Os \
>   -mno-mmx \
>   -mno-sse \
>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>   -c \
>   -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./AutoGen.obj 
> \
>   -IOvmfPkg/Sec/Ia32 \
>   -IOvmfPkg/Sec \
>   -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \
>   -IMdePkg \
>   -IMdePkg/Include \
>   -IMdePkg/Include/Ia32 \
>   -IMdeModulePkg \
>   -IMdeModulePkg/Include \
>   -IUefiCpuPkg \
>   -IUefiCpuPkg/Include \
>   -IOvmfPkg \
>   -IOvmfPkg/Include \
>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c
>
> "ar" \
>   cr \
>   Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/SecMain.lib \
>   @Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/object_files.lst
>
> "gcc" \
>   -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll \
>   -nostdlib \
>   -Wl,-n,-q,--gc-sections \
>   -z common-page-size=0x40 \
>   -Wl,--entry,_ModuleEntryPoint \
>   -u _ModuleEntryPoint \
>   
> -Wl,-Map,Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.map
>  \
>   -Wl,-m,elf_i386,--oformat=elf32-i386 \
>   
> -Wl,--start-group,@Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/static_library_files.lst,--end-group
>  \
>   -g \
>   -fshort-wchar \
>   

Re: [edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Ard Biesheuvel]

On 5 July 2017 at 17:42, Laszlo Ersek  wrote:
> GNU Binutils produce a PE debug directory with one

This sentence already confuses me. This crash is reproducible on ARM,
but the ARM toolchains are strictly ELF based, and all PE/COFF data
structures are created by GenFw itself, never by binutils. So I don't
see how this could be a binutils bug.


> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY:
> - the Type field of the entry is EFI_IMAGE_DEBUG_TYPE_CODEVIEW,
> - the FileOffset field of the entry points right past the entry itself,
> - the data structure placed at FileOffset is a CV_INFO_PDB20 structure,
>   with an "NB10" signature.
>
> This is all correct, except GNU Binutils include the pointed-to
> CV_INFO_PDB20 structure in the size of the debug directory (that is,
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size).
> That's a bug.
>
> The malformed debug directory size causes the loop in GenFw's
> ZeroDebugData() function to process the CV_INFO_PDB20 structure as a set
> of EFI_IMAGE_DEBUG_DIRECTORY_ENTRY elements, which crashes GenFw.
>
> This problem was exposed by commit e4129b0e5897 ("BaseTools: Update GenFw
> to clear unused debug entry generated by VS tool chain", 2017-06-19).
>
> Work around the Binutils issue by noticing when an
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset points back into the debug
> directory. (This can never happen with a well-formed PE file.) In this
> case, truncate DebugDirectoryEntrySize such that the debug directory will
> end right before the debug structure pointed-to by
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset.
>
> Tested with OVMF:
> - gcc-4.8.5-14.el7.x86_64
> - binutils-2.25.1-27.base.el7.x86_64
>
> and with ArmVirtPkg:
> - gcc-aarch64-linux-gnu-6.1.1-2.el7.x86_64
> - binutils-aarch64-linux-gnu-2.27-3.el7.x86_64
>
> Cc: Ard Biesheuvel 
> Cc: Gerd Hoffmann 
> Cc: Leif Lindholm 
> Cc: Liming Gao 
> Cc: Yonghong Zhu 
> Reported-by: Gerd Hoffmann 
> Reported-by: Leif Lindholm 
> Ref: 
> a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com">http://mid.mail-archive.com/a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com
> Ref: 20170705134136.GB26676@bivouac.eciton.net">http://mid.mail-archive.com/20170705134136.GB26676@bivouac.eciton.net
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>
> Notes:
> Repo:   https://github.com/lersek/edk2.git
> Branch: binutils_debugdirsize_workaround
>
>  BaseTools/Source/C/GenFw/GenFw.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c 
> b/BaseTools/Source/C/GenFw/GenFw.c
> index 6569460f34f7..a79f485ee681 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -2771,6 +2771,7 @@ Returns:
>UINT32   Index;
>UINT32   DebugDirectoryEntryRva;
>UINT32   DebugDirectoryEntrySize;
> +  UINT32   TruncatedDebugDirectorySize;
>UINT32   DebugDirectoryEntryFileOffset;
>UINT32   ExportDirectoryEntryRva;
>UINT32   ExportDirectoryEntryFileOffset;
> @@ -2893,6 +2894,25 @@ Returns:
>  DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
> DebugDirectoryEntryFileOffset);
>  Index = 0;
>  for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
> +  //
> +  // Work around GNU Binutils bug: if the debug information pointed-to by
> +  // DebugEntry was incorrectly included in DebugDirectoryEntrySize, then
> +  // the debug directory doesn't actually extend past the pointed-to 
> debug
> +  // information. Truncate DebugDirectoryEntrySize accordingly.
> +  //
> +  if (DebugEntry->FileOffset >= DebugDirectoryEntryFileOffset &&
> +  DebugEntry->FileOffset < (DebugDirectoryEntryFileOffset +
> +DebugDirectoryEntrySize)) {
> +TruncatedDebugDirectorySize = (DebugEntry->FileOffset -
> +   DebugDirectoryEntryFileOffset);
> +VerboseMsg (
> +  "truncating debug directory size from %u to %u",
> +  DebugDirectoryEntrySize,
> +  TruncatedDebugDirectorySize
> +  );
> +DebugDirectoryEntrySize = TruncatedDebugDirectorySize;
> +  }
> +
>DebugEntry->TimeDateStamp = 0;
>if (ZeroDebugFlag || DebugEntry->Type != 
> EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>  memset (FileBuffer + DebugEntry->FileOffset, 0, 
> DebugEntry->SizeOfData);
> --
> 2.13.1.3.g8be5a757fa67
>
___
edk2-devel mailing list
edk2-devel@lists.01.org

[edk2] [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek]

GNU Binutils produce a PE debug directory with one
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY:
- the Type field of the entry is EFI_IMAGE_DEBUG_TYPE_CODEVIEW,
- the FileOffset field of the entry points right past the entry itself,
- the data structure placed at FileOffset is a CV_INFO_PDB20 structure,
  with an "NB10" signature.

This is all correct, except GNU Binutils include the pointed-to
CV_INFO_PDB20 structure in the size of the debug directory (that is,
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size).
That's a bug.

The malformed debug directory size causes the loop in GenFw's
ZeroDebugData() function to process the CV_INFO_PDB20 structure as a set
of EFI_IMAGE_DEBUG_DIRECTORY_ENTRY elements, which crashes GenFw.

This problem was exposed by commit e4129b0e5897 ("BaseTools: Update GenFw
to clear unused debug entry generated by VS tool chain", 2017-06-19).

Work around the Binutils issue by noticing when an
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset points back into the debug
directory. (This can never happen with a well-formed PE file.) In this
case, truncate DebugDirectoryEntrySize such that the debug directory will
end right before the debug structure pointed-to by
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset.

Tested with OVMF:
- gcc-4.8.5-14.el7.x86_64
- binutils-2.25.1-27.base.el7.x86_64

and with ArmVirtPkg:
- gcc-aarch64-linux-gnu-6.1.1-2.el7.x86_64
- binutils-aarch64-linux-gnu-2.27-3.el7.x86_64

Cc: Ard Biesheuvel 
Cc: Gerd Hoffmann 
Cc: Leif Lindholm 
Cc: Liming Gao 
Cc: Yonghong Zhu 
Reported-by: Gerd Hoffmann 
Reported-by: Leif Lindholm 
Ref: a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com">http://mid.mail-archive.com/a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com
Ref: 20170705134136.GB26676@bivouac.eciton.net">http://mid.mail-archive.com/20170705134136.GB26676@bivouac.eciton.net
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
Repo:   https://github.com/lersek/edk2.git
Branch: binutils_debugdirsize_workaround

 BaseTools/Source/C/GenFw/GenFw.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 6569460f34f7..a79f485ee681 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -2771,6 +2771,7 @@ Returns:
   UINT32   Index;
   UINT32   DebugDirectoryEntryRva;
   UINT32   DebugDirectoryEntrySize;
+  UINT32   TruncatedDebugDirectorySize;
   UINT32   DebugDirectoryEntryFileOffset;
   UINT32   ExportDirectoryEntryRva;
   UINT32   ExportDirectoryEntryFileOffset;
@@ -2893,6 +2894,25 @@ Returns:
 DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
DebugDirectoryEntryFileOffset);
 Index = 0;
 for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
+  //
+  // Work around GNU Binutils bug: if the debug information pointed-to by
+  // DebugEntry was incorrectly included in DebugDirectoryEntrySize, then
+  // the debug directory doesn't actually extend past the pointed-to debug
+  // information. Truncate DebugDirectoryEntrySize accordingly.
+  //
+  if (DebugEntry->FileOffset >= DebugDirectoryEntryFileOffset &&
+  DebugEntry->FileOffset < (DebugDirectoryEntryFileOffset +
+DebugDirectoryEntrySize)) {
+TruncatedDebugDirectorySize = (DebugEntry->FileOffset -
+   DebugDirectoryEntryFileOffset);
+VerboseMsg (
+  "truncating debug directory size from %u to %u",
+  DebugDirectoryEntrySize,
+  TruncatedDebugDirectorySize
+  );
+DebugDirectoryEntrySize = TruncatedDebugDirectorySize;
+  }
+
   DebugEntry->TimeDateStamp = 0;
   if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
 memset (FileBuffer + DebugEntry->FileOffset, 0, 
DebugEntry->SizeOfData);
-- 
2.13.1.3.g8be5a757fa67

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

Re: [edk2] [platforms: PATCH v2 10/10] Platform/Marvell: ComPhyLib: Add support for SATA ports on CP110 slave

[Leif Lindholm]

On Wed, Jul 05, 2017 at 12:02:12AM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel 
> 
> Add support for COMPHY_TYPE_SATA2 and COMPHY_TYPE_SATA3, which map
> to the SATA ports on the second CP110's AHCI controller.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> Signed-off-by: Marcin Wojtas 

Reviewed-by: Leif Lindholm 

> ---
>  Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c 
> b/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> index 6ef63a8..40a7b99 100755
> --- a/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> @@ -55,24 +55,26 @@ DECLARE_A7K8K_NONDISCOVERABLE_TEMPLATE;
>  COMPHY_MUX_DATA Cp110ComPhyMuxData[] = {
>/* Lane 0 */
>{4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII1, 0x1},
> -   {COMPHY_TYPE_SATA1, 0x4}}},
> +   {COMPHY_TYPE_SATA1, 0x4}, {COMPHY_TYPE_SATA3, 0x4}}},
>/* Lane 1 */
>{4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII2, 0x1},
> -   {COMPHY_TYPE_SATA0, 0x4}}},
> +   {COMPHY_TYPE_SATA0, 0x4}, {COMPHY_TYPE_SATA2, 0x4}}},
>/* Lane 2 */
>{6, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII0, 0x1},
> {COMPHY_TYPE_RXAUI0, 0x1}, {COMPHY_TYPE_SFI, 0x1},
> -   {COMPHY_TYPE_SATA0, 0x4}}},
> +   {COMPHY_TYPE_SATA0, 0x4}, {COMPHY_TYPE_SATA2, 0x4}}},
>/* Lane 3 */
>{8, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_RXAUI1, 0x1},
> -   {COMPHY_TYPE_SGMII1, 0x2}, {COMPHY_TYPE_SATA1, 0x4}}},
> +   {COMPHY_TYPE_SGMII1, 0x2}, {COMPHY_TYPE_SATA1, 0x4},
> +   {COMPHY_TYPE_SATA3, 0x4}}},
>/* Lane 4 */
>{7, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII0, 0x2},
> {COMPHY_TYPE_RXAUI0, 0x2}, {COMPHY_TYPE_SFI, 0x2},
> {COMPHY_TYPE_SGMII1, 0x1}}},
>/* Lane 5 */
>{6, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII2, 0x1},
> -   {COMPHY_TYPE_RXAUI1, 0x2}, {COMPHY_TYPE_SATA1, 0x4}}},
> +   {COMPHY_TYPE_RXAUI1, 0x2}, {COMPHY_TYPE_SATA1, 0x4},
> +   {COMPHY_TYPE_SATA3, 0x4}}},
>  };
>  
>  COMPHY_MUX_DATA Cp110ComPhyPipeMuxData[] = {
> -- 
> 1.8.3.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH v2 09/10] Platform/Marvell: ComPhyLib: Use COMPHY_ prefix in macros

[Leif Lindholm]

On Wed, Jul 05, 2017 at 12:02:11AM +0200, Marcin Wojtas wrote:
> This patch renames macros for speed, type and polarity from
> 'PHY_' to 'COMPHY_' (Common PHY), so that to avoid confusion
> with network PHY's definitions (NETPHY_).
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marcin Wojtas 

Reviewed-by: Leif Lindholm 

> ---
>  Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c | 102 
> ---
>  Platform/Marvell/Library/ComPhyLib/ComPhyLib.c   |  22 ++---
>  Platform/Marvell/Library/ComPhyLib/ComPhyLib.h   |  90 ++--
>  Platform/Marvell/Library/ComPhyLib/ComPhyMux.c   |   4 +-
>  4 files changed, 111 insertions(+), 107 deletions(-)
> 
> diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c 
> b/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> index 329bbe8..6ef63a8 100755
> --- a/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyCp110.c
> @@ -54,40 +54,44 @@ DECLARE_A7K8K_NONDISCOVERABLE_TEMPLATE;
>   */
>  COMPHY_MUX_DATA Cp110ComPhyMuxData[] = {
>/* Lane 0 */
> -  {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII1, 0x1}, {PHY_TYPE_SATA1, 
> 0x4}}},
> +  {4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII1, 0x1},
> +   {COMPHY_TYPE_SATA1, 0x4}}},
>/* Lane 1 */
> -  {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII2, 0x1}, {PHY_TYPE_SATA0, 
> 0x4}}},
> +  {4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII2, 0x1},
> +   {COMPHY_TYPE_SATA0, 0x4}}},
>/* Lane 2 */
> -  {6, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII0, 0x1}, 
> {PHY_TYPE_RXAUI0, 0x1},
> -{PHY_TYPE_SFI, 0x1}, {PHY_TYPE_SATA0, 0x4}}},
> +  {6, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII0, 0x1},
> +   {COMPHY_TYPE_RXAUI0, 0x1}, {COMPHY_TYPE_SFI, 0x1},
> +   {COMPHY_TYPE_SATA0, 0x4}}},
>/* Lane 3 */
> -  {8, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_RXAUI1, 0x1}, 
> {PHY_TYPE_SGMII1, 0x2},
> -{PHY_TYPE_SATA1, 0x4}}},
> +  {8, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_RXAUI1, 0x1},
> +   {COMPHY_TYPE_SGMII1, 0x2}, {COMPHY_TYPE_SATA1, 0x4}}},
>/* Lane 4 */
> -  {7, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII0, 0x2}, 
> {PHY_TYPE_RXAUI0, 0x2},
> -{PHY_TYPE_SFI, 0x2}, {PHY_TYPE_SGMII1, 0x1}}},
> +  {7, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII0, 0x2},
> +   {COMPHY_TYPE_RXAUI0, 0x2}, {COMPHY_TYPE_SFI, 0x2},
> +   {COMPHY_TYPE_SGMII1, 0x1}}},
>/* Lane 5 */
> -  {6, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII2, 0x1}, 
> {PHY_TYPE_RXAUI1, 0x2},
> -{PHY_TYPE_SATA1, 0x4}}},
> +  {6, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_SGMII2, 0x1},
> +   {COMPHY_TYPE_RXAUI1, 0x2}, {COMPHY_TYPE_SATA1, 0x4}}},
>  };
>  
>  COMPHY_MUX_DATA Cp110ComPhyPipeMuxData[] = {
>/* Lane 0 */
> -  {2, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_PCIE0, 0x4} } },
> +  {2, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_PCIE0, 0x4}}},
>/* Lane 1 */
> -  {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST0, 0x1},
> -{PHY_TYPE_USB3_DEVICE, 0x2}, {PHY_TYPE_PCIE0, 0x4} } },
> +  {4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_USB3_HOST0, 0x1},
> +   {COMPHY_TYPE_USB3_DEVICE, 0x2}, {COMPHY_TYPE_PCIE0, 0x4}}},
>/* Lane 2 */
> -  {3, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST0, 0x1},
> -{PHY_TYPE_PCIE0, 0x4} } },
> +  {3, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_USB3_HOST0, 0x1},
> +   {COMPHY_TYPE_PCIE0, 0x4}}},
>/* Lane 3 */
> -  {3, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST1, 0x1},
> -{PHY_TYPE_PCIE0, 0x4} } },
> +  {3, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_USB3_HOST1, 0x1},
> +   {COMPHY_TYPE_PCIE0, 0x4}}},
>/* Lane 4 */
> -  {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST1, 0x1},
> -{PHY_TYPE_USB3_DEVICE, 0x2}, {PHY_TYPE_PCIE1, 0x4} } },
> +  {4, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_USB3_HOST1, 0x1},
> +   {COMPHY_TYPE_USB3_DEVICE, 0x2}, {COMPHY_TYPE_PCIE1, 0x4}}},
>/* Lane 5 */
> -  {2, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_PCIE2, 0x4} } },
> +  {2, {{COMPHY_TYPE_UNCONNECTED, 0x0}, {COMPHY_TYPE_PCIE2, 0x4}}},
>  };
>  
>  STATIC
> @@ -1102,7 +1106,7 @@ ComPhySgmiiRFUConfiguration (
>Data = 0x0 << SD_EXTERNAL_CONFIG0_SD_PU_PLL_OFFSET;
>Mask |= SD_EXTERNAL_CONFIG0_SD_PHY_GEN_RX_MASK;
>Mask |= SD_EXTERNAL_CONFIG0_SD_PHY_GEN_TX_MASK;
> -  if (SgmiiSpeed == PHY_SPEED_1_25G) {
> +  if (SgmiiSpeed == COMPHY_SPEED_1_25G) {
>  Data |= 0x6 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_RX_OFFSET;
>  Data |= 0x6 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_TX_OFFSET;
>} else {
> @@ -1320,7 +1324,7 @@ ComPhySfiPhyConfiguration (
>  
>/* Set reference clock */
>Mask = HPIPE_MISC_ICP_FORCE_MASK | HPIPE_MISC_REFCLK_SEL_MASK;
> -  Data = (SfiSpeed == PHY_SPEED_5_15625G) ?
> +  Data = (SfiSpeed == COMPHY_SPEED_5_15625G) ?
>  (0x0 << HPIPE_MISC_ICP_FORCE_OFFSET) : (0x1 << 
> HPIPE_MISC_ICP_FORCE_OFFSET);
>

Re: [edk2] [PATCH v2] ArmPlatformPkg: Support different PL011 reg offset

[Leif Lindholm]

On Tue, Jul 04, 2017 at 11:43:38PM +0800, Jun Nie wrote:
> ZTE/SanChip version pl011 has different reg offset and bit offset
> for some registers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie 
> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec  |  1 +
>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf |  1 +
>  ArmPlatformPkg/Include/Drivers/PL011Uart.h | 29 
> ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec 
> b/ArmPlatformPkg/ArmPlatformPkg.dec
> index d756fd2..3dd613c 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -97,6 +97,7 @@
>gArmPlatformTokenSpaceGuid.PL011UartInteger|0|UINT32|0x0020
>gArmPlatformTokenSpaceGuid.PL011UartFractional|0|UINT32|0x002D
>gArmPlatformTokenSpaceGuid.PL011UartInterrupt|0x|UINT32|0x002F
> +  gArmPlatformTokenSpaceGuid.PL011UartZxRegOffset|0|UINT8|0

I'm basically OK with this patch, but if we have multiple variants of
this, as the Linux driver suggests, I think we're looking at something
more like PL011UartRegOffsetVariant, with a numerical value for each
special flavour.

>  
>## PL011 Serial Debug UART
>
> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase|0x|UINT64|0x0030
> diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf 
> b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf
> index 0154f3b..257fbc7 100644
> --- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf
> +++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf
> @@ -39,3 +39,4 @@
>  
>gArmPlatformTokenSpaceGuid.PL011UartInteger
>gArmPlatformTokenSpaceGuid.PL011UartFractional
> +  gArmPlatformTokenSpaceGuid.PL011UartZxRegOffset
> diff --git a/ArmPlatformPkg/Include/Drivers/PL011Uart.h 
> b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> index d5e88e8..09d548b 100644
> --- a/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> +++ b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> @@ -19,6 +19,7 @@
>  #include 
>  
>  // PL011 Registers
> +#if !FixedPcdGet8 (PL011UartZxRegOffset)

And as such, more a test like...
#if FixedPcdGet8 (PL011UartRegOffsetVariant) == PL011_VARIANT_ZTE

/
Leif

>  #define UARTDR0x000
>  #define UARTRSR   0x004
>  #define UARTECR   0x004
> @@ -34,6 +35,22 @@
>  #define UARTMIS   0x040
>  #define UARTICR   0x044
>  #define UARTDMACR 0x048
> +#else
> +#define UARTDR0x004
> +#define UARTRSR   0x010
> +#define UARTECR   0x010
> +#define UARTFR0x014
> +#define UARTIBRD  0x024
> +#define UARTFBRD  0x028
> +#define UARTLCR_H 0x030
> +#define UARTCR0x034
> +#define UARTIFLS  0x038
> +#define UARTIMSC  0x040
> +#define UARTRIS   0x044
> +#define UARTMIS   0x048
> +#define UARTICR   0x04c
> +#define UARTDMACR 0x050
> +#endif
>  
>  #define UARTPID0  0xFE0
>  #define UARTPID1  0xFE4
> @@ -47,6 +64,7 @@
>  #define UART_STATUS_ERROR_MASK0x0F
>  
>  // Flag reg bits
> +#if !FixedPcdGet8 (PL011UartZxRegOffset)
>  #define PL011_UARTFR_RI   (1 << 8)  // Ring indicator
>  #define PL011_UARTFR_TXFE (1 << 7)  // Transmit FIFO empty
>  #define PL011_UARTFR_RXFF (1 << 6)  // Receive  FIFO full
> @@ -56,6 +74,17 @@
>  #define PL011_UARTFR_DCD  (1 << 2)  // Data carrier detect
>  #define PL011_UARTFR_DSR  (1 << 1)  // Data set ready
>  #define PL011_UARTFR_CTS  (1 << 0)  // Clear to send
> +#else
> +#define PL011_UARTFR_RI   (1 << 0)  // Ring indicator
> +#define PL011_UARTFR_TXFE (1 << 7)  // Transmit FIFO empty
> +#define PL011_UARTFR_RXFF (1 << 6)  // Receive  FIFO full
> +#define PL011_UARTFR_TXFF (1 << 5)  // Transmit FIFO full
> +#define PL011_UARTFR_RXFE (1 << 4)  // Receive  FIFO empty
> +#define PL011_UARTFR_BUSY (1 << 8)  // UART busy
> +#define PL011_UARTFR_DCD  (1 << 2)  // Data carrier detect
> +#define PL011_UARTFR_DSR  (1 << 3)  // Data set ready
> +#define PL011_UARTFR_CTS  (1 << 1)  // Clear to send
> +#endif
>  
>  // Flag reg bits - alternative names
>  #define UART_TX_EMPTY_FLAG_MASK   PL011_UARTFR_TXFE
> -- 
> 1.9.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 16:07, Laszlo Ersek  wrote:
> On 07/05/17 17:01, Ard Biesheuvel wrote:
>> On 5 July 2017 at 15:25, Laszlo Ersek  wrote:
>>> On 07/05/17 15:46, Ard Biesheuvel wrote:
 On 5 July 2017 at 14:46, Ard Biesheuvel  wrote:
> On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>> in MdeModulePkg.
>>>
>>> However, as it turns out, the generic driver has a dependency on the
>>> library class ReportStatusCodeLib, whose default resolution is an
>>> implementation that is not safe for use at runtime, resulting in crashes
>>> when trying to invoke it from the OS.
>>>
>>> Since we have no use for status codes in any of the ArmVirtPkg
>>> platforms, let's replace all resolutions with a common one to the NULL
>>> implementation.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> Alternative approach (if we wish to follow what OVMF does):
>>
>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>
>> - SEC, PEI_CORE, PEIM:
>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>
>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>
>> - DXE_RUNTIME_DRIVER:
>>   
>> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>
>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and 
>> Handler from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>
>> This should result in status code reporting and handling that is 
>> functional at runtime as well.
>>
>> What do you prefer?
>>


 Depends on what status codes actually buy us. I'm clueless

>>>
>>> I was similarly clueless :) which is why I asked Cinnamon back then [*]
>>> to write a detailed commit message for what would become commit
>>> a6d594c5fabd.
>>>
>>> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>>>
>>> So, if you read that message, you'll see that roughly, it buys us the
>>> following in practice: when a module (boot time or runtime) calls one of
>>> the REPORT_STATUS_CODE*() macros, a status code message will be printed
>>> to the serial port.
>>>
>>> That's it :)
>>>
>>> I don't think it's extremely useful in practice for in-tree modules,
>>> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
>>> modules with high DEBUG levels, add DEBUGs liberally to our own platform
>>> code, and always capture the DEBUG output (serial port or QEMU debug
>>> port) during boot.
>>>
>>> For working with out-of-tree modules that rely heavily on status code
>>> reporting, or else just to showcase/test the related PPIs and protocols
>>> (which is arguably one of the goals of the in-tree virt platforms),
>>> enabling the status code libs and drivers can be considered useful.
>>>
>>> Given that OvmfPkg is already serving this showcase/test purpose, I'm
>>> fine with disabling status code stuff in ArmVirtPkg for good; I just
>>> wanted you to consider the alternative. So, what say you? :)
>>>
>>
>> Thanks for the explanation. I think one showcase is sufficient, indeed.
>>
>
> In that case :)
>
> Reviewed-by: Laszlo Ersek 
>

Thanks!

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

Re: [edk2] [PATCH v2] ArmPlatformPkg: convert VExpress ResetSystemLib to ResetSystemLib

[Leif Lindholm]

On Wed, Jul 05, 2017 at 12:20:14PM +0100, Ryan Harkin wrote:
> On 4 July 2017 at 18:27, Ard Biesheuvel  wrote:
> > On 4 July 2017 at 18:11, Leif Lindholm  wrote:
> >> Since we're in the process of migrating all of the VExpress platforms
> >> to MdeModulePkg ResetSystemRuntimeDxe, convert VExpress ResetSystemLib
> >> from EfiResetSystemLib interface to the ResetSystemLib one.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Leif Lindholm 
> >
> > Reviewed-by: Ard Biesheuvel 
> 
> After popping commit e4129b0e5897d76885170bec9da996b266f185f9 off the
> top of edk2, and adding this, I also applied patch "Platforms/ARM:
> move ARM platforms to generic ResetSystemRuntimeDxe" to
> OpenPlatformPkg and tested on TC2, FVP Foundation and AEMv8 models and
> Juno R0/1/2.
> 
> I checked that reset and shutdown both still work as expected. Reset
> was triggered from BDS, grub and EFI Shell. Shutdown (reset -s) was
> trigger from EFI Shell.
> 
> Tested-by: Ryan Harkin 

Thanks guys.
This end pushed as 729ddffda0.

> >
> >> ---
> >>  .../Library/ResetSystemLib/ResetSystemLib.c| 117 
> >> -
> >>  .../Library/ResetSystemLib/ResetSystemLib.inf  |   6 +-
> >>  2 files changed, 73 insertions(+), 50 deletions(-)
> >>
> >> diff --git 
> >> a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c 
> >> b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> >> index bafb6f8093..d2bc4a88fa 100644
> >> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> >> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> >> @@ -5,6 +5,7 @@
> >>
> >>Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> >>Copyright (c) 2013, ARM Ltd. All rights reserved.
> >> +  Copyright (c) 2017, Linaro Ltd. All rights reserved.
> >>
> >>This program and the accompanying materials
> >>are licensed and made available under the terms and conditions of the 
> >> BSD License
> >> @@ -16,73 +17,95 @@
> >>
> >>  **/
> >>
> >> -#include 
> >> +#include 
> >>
> >> -#include 
> >> -#include 
> >> -#include 
> >>  #include 
> >> +#include 
> >> +#include 
> >>
> >>  #include 
> >>
> >>  /**
> >> -  Resets the entire platform.
> >> +  This function causes a system-wide reset (cold reset), in which
> >> +  all circuitry within the system returns to its initial state. This type 
> >> of
> >> +  reset is asynchronous to system operation and operates without regard to
> >> +  cycle boundaries.
> >>
> >> -  @param  ResetType The type of reset to perform.
> >> -  @param  ResetStatus   The status code for the reset.
> >> -  @param  DataSize  The size, in bytes, of WatchdogData.
> >> -  @param  ResetData For a ResetType of EfiResetCold, 
> >> EfiResetWarm, or
> >> -EfiResetShutdown the data buffer starts 
> >> with a Null-terminated
> >> -Unicode string, optionally followed by 
> >> additional binary data.
> >> +  If this function returns, it means that the system does not support cold
> >> +  reset.
> >> +**/
> >> +VOID
> >> +EFIAPI
> >> +ResetCold (
> >> +  VOID
> >> +  )
> >> +{
> >> +  ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
> >> +}
> >> +
> >> +/**
> >> +  This function causes a system-wide initialization (warm reset), in 
> >> which all
> >> +  processors are set to their initial state. Pending cycles are not 
> >> corrupted.
> >>
> >> +  If this function returns, it means that the system does not support warm
> >> +  reset.
> >>  **/
> >> -EFI_STATUS
> >> +VOID
> >>  EFIAPI
> >> -LibResetSystem (
> >> -  IN EFI_RESET_TYPE   ResetType,
> >> -  IN EFI_STATUS   ResetStatus,
> >> -  IN UINTNDataSize,
> >> -  IN CHAR16   *ResetData OPTIONAL
> >> +ResetWarm (
> >> +  VOID
> >>)
> >>  {
> >> -  switch (ResetType) {
> >> -  case EfiResetPlatformSpecific:
> >> -// Map the platform specific reset as reboot
> >> -  case EfiResetWarm:
> >> -// Map a warm reset into a cold reset
> >> -  case EfiResetCold:
> >> -// Send the REBOOT function to the platform microcontroller
> >> -ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
> >> -
> >> -// We should never be here
> >> -while(1);
> >> -  case EfiResetShutdown:
> >> -// Send the SHUTDOWN function to the platform microcontroller
> >> -ArmPlatformSysConfigSet (SYS_CFG_SHUTDOWN, 0);
> >> -
> >> -// We should never be here
> >> -while(1);
> >> -  }
> >> -
> >> -  ASSERT(FALSE);
> >> -  return EFI_UNSUPPORTED;
> >> +  ResetCold ();
> >>  }
> >>
> >>  /**
> >> -  Initialize any infrastructure required for LibResetSystem () to 
> >> function.
> >> +  This function causes the system to enter a power state equivalent
> >> +  to the ACPI G2/S5 or G3 states.
> >> +
> >> +  

Re: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

[Laszlo Ersek]

I understand it now. The article at
 was of great help.

On 07/05/17 15:29, Laszlo Ersek wrote:
> On 07/05/17 14:42, Laszlo Ersek wrote:
>> On 07/03/17 07:21, Liming Gao wrote:

>>> @@ -2886,11 +2891,23 @@ Returns:
>>>
>>>if (DebugDirectoryEntryFileOffset != 0) {
>>>  DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
>>> DebugDirectoryEntryFileOffset);
>>> -DebugEntry->TimeDateStamp = 0;
>>> -mImageTimeStamp = 0;
>>> -if (ZeroDebugFlag) {
>>> -  memset (FileBuffer + DebugEntry->FileOffset, 0, 
>>> DebugEntry->SizeOfData);
>>> -  memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>>> +Index = 0;
>>> +for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
>>> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
>>> +  DebugEntry->TimeDateStamp = 0;
>>> +  if (ZeroDebugFlag || DebugEntry->Type != 
>>> EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>>> +memset (FileBuffer + DebugEntry->FileOffset, 0, 
>>> DebugEntry->SizeOfData);
>>
>> This memset() is the culprit.
>>
>> According to gdb (all values decimal),
>> - Index = 1,
>> - DebugDirectoryEntrySize = 237,
>> - ZeroDebugFlag = 0.
>>
>> These values look suspicious, because
>> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
>> DebugDirectoryEntrySize (237) is not an integral multiple of that.
>>
>> This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
>>
>>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
>>> DebugDirectoryEntryFileOffset))[0]
>>> $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, 
>>> MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 
>>> 33292}

Notice this. The first entry (which is valid, with type =
EFI_IMAGE_DEBUG_TYPE_CODEVIEW), points at offset 33292, and the
pointed-to data (which is a CodeView information block) has size 209.
This means that the first byte *past* the CodeView information is at
offset 33292 + 209 = 33501.

Now, look at the data dictionary again:

>>> (gdb) print Optional64Hdr->DataDirectory
>>> $22 = {{VirtualAddress = 0, Size = 0},// 
>>> EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
>>>{VirtualAddress = 0, Size = 0},// 
>>> EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
>>>{VirtualAddress = 0, Size = 0},// 
>>> EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
>>>{VirtualAddress = 0, Size = 0},// 
>>> EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
>>>{VirtualAddress = 0, Size = 0},// 
>>> EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
>>>{VirtualAddress = 36864, Size = 4096}, // 
>>> EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
>>>{VirtualAddress = 33264, Size = 237},  // 
>>> EFI_IMAGE_DIRECTORY_ENTRY_DEBUG

Notice this: 33264 + 237 = 33501. The same end offset!

And then: 237 - 209 = 28, which is exactly the size of
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.

So, what happens is that the GNU linker (or some other bin-util that
produces the input file for GenFw)

(1) creates a valid EFI_IMAGE_DEBUG_DIRECTORY_ENTRY, of type
EFI_IMAGE_DEBUG_TYPE_CODEVIEW, with size = 28,

(2) this element points to a valid CV_INFO_PDB20 structure (having NB10
for signature), with size = 209,

(3) The CV_INFO_PDB20 structure *immediately* follows the
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element that points to it -- this is all
fine,

(4) *however*, the pointed-to CV_INFO_PDB20 element (from (3)) is *also*
included in the size of the debug directory, even though the size of the
debug directory should *only* describe the debug directory entry from (1)!

So, this is most certainly a GNU Binutils bug.

We can catch it though: as soon as we find an
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY whose FileOffset field points into the
debug directory itself, we know that the debug directory's size has to
be truncated at once to that offset.

Let me see if I can write a patch for this.

Thanks
Laszlo

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

Re: [edk2] [PATCH v5] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer

[Leif Lindholm]

On Tue, Jul 04, 2017 at 11:43:16PM +0800, Jun Nie wrote:
> ExtCSD structure may be read via DMA. So align it to
> page to avoid data corruption.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie 

Reviewed-by: Leif Lindholm 
Pushed as 7bb5fad566.

> ---
>  EmbeddedPkg/Universal/MmcDxe/Mmc.c   |  3 +++
>  EmbeddedPkg/Universal/MmcDxe/Mmc.h   |  2 +-
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 19 ++-
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.c 
> b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> index 30268e3..ce27150 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> @@ -177,6 +177,9 @@ EFI_STATUS DestroyMmcHostInstance (
>if (MmcHostInstance->BlockIo.Media) {
>  FreePool(MmcHostInstance->BlockIo.Media);
>}
> +  if (MmcHostInstance->CardInfo.ECSDData) {
> +FreePages (MmcHostInstance->CardInfo.ECSDData, EFI_SIZE_TO_PAGES (sizeof 
> (ECSD)));
> +  }
>FreePool (MmcHostInstance);
>  
>return Status;
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h 
> b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> index b7b9454..eb4652b 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> @@ -320,7 +320,7 @@ typedef struct  {
>OCR   OCRData;
>CID   CIDData;
>CSD   CSDData;
> -  ECSD  ECSDData; // MMC V4 extended card 
> specific
> +  ECSD  *ECSDData; // MMC V4 extended card 
> specific
>  } CARD_INFO;
>  
>  typedef struct _MMC_HOST_INSTANCE {
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c 
> b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index c28207e..7f74c54 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -13,6 +13,7 @@
>  **/
>  
>  #include 
> +#include 
>  #include 
>  
>  #include "Mmc.h"
> @@ -210,15 +211,19 @@ EmmcIdentificationMode (
>}
>  
>// Fetch ECSD
> +  MmcHostInstance->CardInfo.ECSDData = AllocatePages (EFI_SIZE_TO_PAGES 
> (sizeof (ECSD)));
> +  if (MmcHostInstance->CardInfo.ECSDData == NULL) {
> +return EFI_OUT_OF_RESOURCES;
> +  }
>Status = Host->SendCommand (Host, MMC_CMD8, 0);
>if (EFI_ERROR (Status)) {
>  DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, 
> Status=%r.\n", Status));
>}
>  
> -  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 
> *)&(MmcHostInstance->CardInfo.ECSDData));
> +  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 
> *)MmcHostInstance->CardInfo.ECSDData);
>if (EFI_ERROR (Status)) {
>  DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, 
> Status=%r.\n", Status));
> -return Status;
> +goto FreePageExit;
>}
>  
>// Make sure device exiting data mode
> @@ -226,7 +231,7 @@ EmmcIdentificationMode (
>  Status = EmmcGetDeviceState (MmcHostInstance, );
>  if (EFI_ERROR (Status)) {
>DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Failed to get device 
> state, Status=%r.\n", Status));
> -  return Status;
> +  goto FreePageExit;
>  }
>} while (State == EMMC_DATA_STATE);
>  
> @@ -237,12 +242,16 @@ EmmcIdentificationMode (
>Media->LogicalBlocksPerPhysicalBlock = 1;
>Media->IoAlign = 4;
>// Compute last block using bits [215:212] of the ECSD
> -  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // 
> eMMC isn't supposed to report this for
> +  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData->SECTOR_COUNT - 1; 
> // eMMC isn't supposed to report this for
>// Cards <2GB in size, but the model does.
>  
>// Setup card type
>MmcHostInstance->CardInfo.CardType = EMMC_CARD;
>return EFI_SUCCESS;
> +
> +FreePageExit:
> +  FreePages (MmcHostInstance->CardInfo.ECSDData, EFI_SIZE_TO_PAGES (sizeof 
> (ECSD)));
> +  return Status;
>  }
>  
>  STATIC
> @@ -258,7 +267,7 @@ InitializeEmmcDevice (
>UINT32 TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, 
> EMMCHS26};
>  
>Host  = MmcHostInstance->MmcHost;
> -  ECSDData = >CardInfo.ECSDData;
> +  ECSDData = MmcHostInstance->CardInfo.ECSDData;
>if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)
>  return EFI_SUCCESS;
>  
> -- 
> 1.9.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 17:01, Ard Biesheuvel wrote:
> On 5 July 2017 at 15:25, Laszlo Ersek  wrote:
>> On 07/05/17 15:46, Ard Biesheuvel wrote:
>>> On 5 July 2017 at 14:46, Ard Biesheuvel  wrote:
 On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
> On 07/05/17 15:04, Ard Biesheuvel wrote:
>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>> from EmbeddedPkg with the well maintained generic alternative that lives
>> in MdeModulePkg.
>>
>> However, as it turns out, the generic driver has a dependency on the
>> library class ReportStatusCodeLib, whose default resolution is an
>> implementation that is not safe for use at runtime, resulting in crashes
>> when trying to invoke it from the OS.
>>
>> Since we have no use for status codes in any of the ArmVirtPkg
>> platforms, let's replace all resolutions with a common one to the NULL
>> implementation.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> Alternative approach (if we wish to follow what OVMF does):
>
> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>
> - SEC, PEI_CORE, PEIM:
>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>
> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> - DXE_RUNTIME_DRIVER:
>   
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>
> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler 
> from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>
> This should result in status code reporting and handling that is 
> functional at runtime as well.
>
> What do you prefer?
>
>>>
>>>
>>> Depends on what status codes actually buy us. I'm clueless
>>>
>>
>> I was similarly clueless :) which is why I asked Cinnamon back then [*]
>> to write a detailed commit message for what would become commit
>> a6d594c5fabd.
>>
>> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>>
>> So, if you read that message, you'll see that roughly, it buys us the
>> following in practice: when a module (boot time or runtime) calls one of
>> the REPORT_STATUS_CODE*() macros, a status code message will be printed
>> to the serial port.
>>
>> That's it :)
>>
>> I don't think it's extremely useful in practice for in-tree modules,
>> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
>> modules with high DEBUG levels, add DEBUGs liberally to our own platform
>> code, and always capture the DEBUG output (serial port or QEMU debug
>> port) during boot.
>>
>> For working with out-of-tree modules that rely heavily on status code
>> reporting, or else just to showcase/test the related PPIs and protocols
>> (which is arguably one of the goals of the in-tree virt platforms),
>> enabling the status code libs and drivers can be considered useful.
>>
>> Given that OvmfPkg is already serving this showcase/test purpose, I'm
>> fine with disabling status code stuff in ArmVirtPkg for good; I just
>> wanted you to consider the alternative. So, what say you? :)
>>
> 
> Thanks for the explanation. I think one showcase is sufficient, indeed.
> 

In that case :)

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 15:25, Laszlo Ersek  wrote:
> On 07/05/17 15:46, Ard Biesheuvel wrote:
>> On 5 July 2017 at 14:46, Ard Biesheuvel  wrote:
>>> On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
 On 07/05/17 15:04, Ard Biesheuvel wrote:
> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
> from EmbeddedPkg with the well maintained generic alternative that lives
> in MdeModulePkg.
>
> However, as it turns out, the generic driver has a dependency on the
> library class ReportStatusCodeLib, whose default resolution is an
> implementation that is not safe for use at runtime, resulting in crashes
> when trying to invoke it from the OS.
>
> Since we have no use for status codes in any of the ArmVirtPkg
> platforms, let's replace all resolutions with a common one to the NULL
> implementation.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>  1 file changed, 2 insertions(+), 9 deletions(-)

 Alternative approach (if we wish to follow what OVMF does):

 (1) Copy the library resolutions (as appropriate) from OvmfPkg:

 - SEC, PEI_CORE, PEIM:
   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf

 - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

 - DXE_RUNTIME_DRIVER:
   
 MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf

 (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler 
 from MdeModulePkg", 2016-08-03) to ArmVirtPkg.

 This should result in status code reporting and handling that is 
 functional at runtime as well.

 What do you prefer?

>>
>>
>> Depends on what status codes actually buy us. I'm clueless
>>
>
> I was similarly clueless :) which is why I asked Cinnamon back then [*]
> to write a detailed commit message for what would become commit
> a6d594c5fabd.
>
> [*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html
>
> So, if you read that message, you'll see that roughly, it buys us the
> following in practice: when a module (boot time or runtime) calls one of
> the REPORT_STATUS_CODE*() macros, a status code message will be printed
> to the serial port.
>
> That's it :)
>
> I don't think it's extremely useful in practice for in-tree modules,
> given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
> modules with high DEBUG levels, add DEBUGs liberally to our own platform
> code, and always capture the DEBUG output (serial port or QEMU debug
> port) during boot.
>
> For working with out-of-tree modules that rely heavily on status code
> reporting, or else just to showcase/test the related PPIs and protocols
> (which is arguably one of the goals of the in-tree virt platforms),
> enabling the status code libs and drivers can be considered useful.
>
> Given that OvmfPkg is already serving this showcase/test purpose, I'm
> fine with disabling status code stuff in ArmVirtPkg for good; I just
> wanted you to consider the alternative. So, what say you? :)
>

Thanks for the explanation. I think one showcase is sufficient, indeed.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 15:46, Ard Biesheuvel wrote:
> On 5 July 2017 at 14:46, Ard Biesheuvel  wrote:
>> On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
>>> On 07/05/17 15:04, Ard Biesheuvel wrote:
 Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
 replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
 from EmbeddedPkg with the well maintained generic alternative that lives
 in MdeModulePkg.

 However, as it turns out, the generic driver has a dependency on the
 library class ReportStatusCodeLib, whose default resolution is an
 implementation that is not safe for use at runtime, resulting in crashes
 when trying to invoke it from the OS.

 Since we have no use for status codes in any of the ArmVirtPkg
 platforms, let's replace all resolutions with a common one to the NULL
 implementation.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ard Biesheuvel 
 ---
  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)
>>>
>>> Alternative approach (if we wish to follow what OVMF does):
>>>
>>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>>
>>> - SEC, PEI_CORE, PEIM:
>>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>>
>>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>
>>> - DXE_RUNTIME_DRIVER:
>>>   
>>> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>>
>>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler 
>>> from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>>
>>> This should result in status code reporting and handling that is functional 
>>> at runtime as well.
>>>
>>> What do you prefer?
>>>
> 
> 
> Depends on what status codes actually buy us. I'm clueless
> 

I was similarly clueless :) which is why I asked Cinnamon back then [*]
to write a detailed commit message for what would become commit
a6d594c5fabd.

[*] https://lists.01.org/pipermail/edk2-devel/2016-August/000199.html

So, if you read that message, you'll see that roughly, it buys us the
following in practice: when a module (boot time or runtime) calls one of
the REPORT_STATUS_CODE*() macros, a status code message will be printed
to the serial port.

That's it :)

I don't think it's extremely useful in practice for in-tree modules,
given that we -- ArmVirtPkg and OvmfPkg users -- prefer building all
modules with high DEBUG levels, add DEBUGs liberally to our own platform
code, and always capture the DEBUG output (serial port or QEMU debug
port) during boot.

For working with out-of-tree modules that rely heavily on status code
reporting, or else just to showcase/test the related PPIs and protocols
(which is arguably one of the goals of the in-tree virt platforms),
enabling the status code libs and drivers can be considered useful.

Given that OvmfPkg is already serving this showcase/test purpose, I'm
fine with disabling status code stuff in ArmVirtPkg for good; I just
wanted you to consider the alternative. So, what say you? :)

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

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 14:46, Ard Biesheuvel  wrote:
> On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
>> On 07/05/17 15:04, Ard Biesheuvel wrote:
>>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>>> from EmbeddedPkg with the well maintained generic alternative that lives
>>> in MdeModulePkg.
>>>
>>> However, as it turns out, the generic driver has a dependency on the
>>> library class ReportStatusCodeLib, whose default resolution is an
>>> implementation that is not safe for use at runtime, resulting in crashes
>>> when trying to invoke it from the OS.
>>>
>>> Since we have no use for status codes in any of the ArmVirtPkg
>>> platforms, let's replace all resolutions with a common one to the NULL
>>> implementation.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> Alternative approach (if we wish to follow what OVMF does):
>>
>> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>>
>> - SEC, PEI_CORE, PEIM:
>>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>
>> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>
>> - DXE_RUNTIME_DRIVER:
>>   
>> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>>
>> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler 
>> from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>>
>> This should result in status code reporting and handling that is functional 
>> at runtime as well.
>>
>> What do you prefer?
>>


Depends on what status codes actually buy us. I'm clueless
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

On 5 July 2017 at 14:43, Laszlo Ersek  wrote:
> On 07/05/17 15:04, Ard Biesheuvel wrote:
>> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
>> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
>> from EmbeddedPkg with the well maintained generic alternative that lives
>> in MdeModulePkg.
>>
>> However, as it turns out, the generic driver has a dependency on the
>> library class ReportStatusCodeLib, whose default resolution is an
>> implementation that is not safe for use at runtime, resulting in crashes
>> when trying to invoke it from the OS.
>>
>> Since we have no use for status codes in any of the ArmVirtPkg
>> platforms, let's replace all resolutions with a common one to the NULL
>> implementation.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> Alternative approach (if we wish to follow what OVMF does):
>
> (1) Copy the library resolutions (as appropriate) from OvmfPkg:
>
> - SEC, PEI_CORE, PEIM:
>   MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>
> - DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
>   MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> - DXE_RUNTIME_DRIVER:
>   
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>
> (2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler 
> from MdeModulePkg", 2016-08-03) to ArmVirtPkg.
>
> This should result in status code reporting and handling that is functional 
> at runtime as well.
>
> What do you prefer?
>
> Thanks
> Laszlo
>
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 0b9b457b5619..bfc40286d7ab 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -154,6 +154,8 @@ [LibraryClasses.common]
>>VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>>
>> UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>>
>> +  
>> ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
>> +
>>  [LibraryClasses.common.SEC]
>>PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>>BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>> @@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
>>
>> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>>PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
>>
>> PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
>> -  
>> ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>
>> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>>
>> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>>
>> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>> @@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
>>
>> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>>PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>>
>> PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
>> -  
>> ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>>
>> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>>
>> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>>
>> PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
>> @@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
>>HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>>
>> MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>>DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>> -  
>> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>
>> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>>
>> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>>
>> PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
>>
>>  [LibraryClasses.common.DXE_DRIVER]
>> -  
>> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>>
>> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
>>
>> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>>
>> 

[edk2] [platforms: PATCH] Platform/Marvell/Armada: Remove status code support

[Marcin Wojtas]

Switching to the generic ResetSystemRuntimeDxe introduces
dependency on the library class ReportStatusCodeLib, which
is harmful to be called in the runtime from OS, resulting
in crashes.

Safely switch to the NULL implementation, beacause status
codes are unused on Armada platforms.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marcin Wojtas 
Signed-off-by: Ard Biesheuvel 
---
 Platform/Marvell/Armada/Armada.dsc.inc | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/Platform/Marvell/Armada/Armada.dsc.inc 
b/Platform/Marvell/Armada/Armada.dsc.inc
index 3c6ed22..fd42c2e 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -61,7 +61,7 @@
   
UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   
DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
-  
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
+  
ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
   
UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   
UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
   HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
@@ -161,14 +161,12 @@
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
   
MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   
ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   
PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
 
 [LibraryClasses.common.DXE_DRIVER]
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   
SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
@@ -182,7 +180,6 @@
   HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
 
 [LibraryClasses.common.UEFI_DRIVER]
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   
UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
   
ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
@@ -191,7 +188,6 @@
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
 
 [LibraryClasses.ARM, LibraryClasses.AARCH64]
@@ -326,7 +322,6 @@
 !else
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x800F
 !endif
-  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
 
   gEmbeddedTokenSpaceGuid.PcdEmbeddedAutomaticBootCommand|""
   gEmbeddedTokenSpaceGuid.PcdEmbeddedDefaultTextColor|0x07
-- 
1.8.3.1

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

Re: [edk2] [PATCH] ArmVirtPkg: remove status code support

[Laszlo Ersek]

On 07/05/17 15:04, Ard Biesheuvel wrote:
> Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
> replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
> from EmbeddedPkg with the well maintained generic alternative that lives
> in MdeModulePkg.
> 
> However, as it turns out, the generic driver has a dependency on the
> library class ReportStatusCodeLib, whose default resolution is an
> implementation that is not safe for use at runtime, resulting in crashes
> when trying to invoke it from the OS.
> 
> Since we have no use for status codes in any of the ArmVirtPkg
> platforms, let's replace all resolutions with a common one to the NULL
> implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
>  1 file changed, 2 insertions(+), 9 deletions(-)

Alternative approach (if we wish to follow what OVMF does):

(1) Copy the library resolutions (as appropriate) from OvmfPkg:

- SEC, PEI_CORE, PEIM:
  MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf

- DXE_CORE, DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION:
  MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

- DXE_RUNTIME_DRIVER:
  
MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf

(2) Port commit a6d594c5fabd ("OvmfPkg: use StatusCode Router and Handler from 
MdeModulePkg", 2016-08-03) to ArmVirtPkg.

This should result in status code reporting and handling that is functional at 
runtime as well.

What do you prefer?

Thanks
Laszlo

> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 0b9b457b5619..bfc40286d7ab 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -154,6 +154,8 @@ [LibraryClasses.common]
>VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>
> UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>  
> +  
> ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> +
>  [LibraryClasses.common.SEC]
>PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> @@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
>
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
>PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
> -  
> ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
> @@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
>
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
> -  
> ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
>
> PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
> @@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
>HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>
> MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> -  
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>
> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
>
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
>
> PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
>  
>  [LibraryClasses.common.DXE_DRIVER]
> -  
> ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
>
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
>PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -223,10 +221,8 @@ [LibraryClasses.common.UEFI_APPLICATION]
>HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
>

Re: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

[Leif Lindholm]

Many thanks for tracking this down Laszlo - we'd stumbled over it
ourselves this morning.

Liming, Mike, Andrew: this currently makes the BaseTools unusable with
gcc toolchains. Can we revert this commit until this has been
resolved?

Best Regards,

Leif

On Wed, Jul 05, 2017 at 02:42:15PM +0200, Laszlo Ersek wrote:
> Hi Liming,
> 
> Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
> 
> I can reproduce the segfault locally. This is the command line:
> 
> > GenFw \
> >   -e DXE_RUNTIME_DRIVER \
> >   -o 
> > .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi
> >  \
> >   
> > Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
> 
> Backtrace from gdb:
> 
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
> > Missing separate debuginfos, use: debuginfo-install 
> > glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
> > (gdb) where
> > #0  0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
> > #1  0x00407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", 
> > ZeroDebugFlag=0 '\000') at GenFw.c:2898
> > #2  0x00406a4d in main (argc=0, argv=0x7fffd0b8) at GenFw.c:2591
> 
> See below for the SIGSEGV location:
> 
> On 07/03/17 07:21, Liming Gao wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=600
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Liming Gao 
> > ---
> >  BaseTools/Source/C/GenFw/GenFw.c | 27 ++-
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/BaseTools/Source/C/GenFw/GenFw.c 
> > b/BaseTools/Source/C/GenFw/GenFw.c
> > index 22e4e72..6569460 100644
> > --- a/BaseTools/Source/C/GenFw/GenFw.c
> > +++ b/BaseTools/Source/C/GenFw/GenFw.c
> > @@ -2770,6 +2770,7 @@ Returns:
> >  {
> >UINT32   Index;
> >UINT32   DebugDirectoryEntryRva;
> > +  UINT32   DebugDirectoryEntrySize;
> >UINT32   DebugDirectoryEntryFileOffset;
> >UINT32   ExportDirectoryEntryRva;
> >UINT32   ExportDirectoryEntryFileOffset;
> > @@ -2781,12 +2782,14 @@ Returns:
> >EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
> >EFI_IMAGE_SECTION_HEADER*SectionHeader;
> >EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
> > +  EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
> >UINT32  *NewTimeStamp;
> >
> >//
> >// Init variable.
> >//
> >DebugDirectoryEntryRva   = 0;
> > +  DebugDirectoryEntrySize  = 0;
> >ExportDirectoryEntryRva  = 0;
> >ResourceDirectoryEntryRva= 0;
> >DebugDirectoryEntryFileOffset= 0;
> > @@ -2822,6 +2825,7 @@ Returns:
> >  if (Optional32Hdr->NumberOfRvaAndSizes > 
> > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> >  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> > != 0) {
> >DebugDirectoryEntryRva = 
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> > +  DebugDirectoryEntrySize = 
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> >if (ZeroDebugFlag) {
> >  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> > = 0;
> >  
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress
> >  = 0;
> > @@ -2841,6 +2845,7 @@ Returns:
> >  if (Optional64Hdr->NumberOfRvaAndSizes > 
> > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> >  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> > != 0) {
> >DebugDirectoryEntryRva = 
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> > +  DebugDirectoryEntrySize = 
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> >if (ZeroDebugFlag) {
> >  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> > = 0;
> >  
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress
> >  = 0;
> > @@ -2886,11 +2891,23 @@ Returns:
> >
> >if (DebugDirectoryEntryFileOffset != 0) {
> >  DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
> > DebugDirectoryEntryFileOffset);
> > -DebugEntry->TimeDateStamp = 0;
> > -mImageTimeStamp = 0;
> > -if (ZeroDebugFlag) {
> > -  memset (FileBuffer + DebugEntry->FileOffset, 0, 
> > DebugEntry->SizeOfData);
> > -  memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> > +Index = 0;
> > +for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
> > (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, 

Re: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

[Laszlo Ersek]

On 07/05/17 14:42, Laszlo Ersek wrote:
> Hi Liming,
> 
> Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
> 
> I can reproduce the segfault locally. This is the command line:
> 
>> GenFw \
>>   -e DXE_RUNTIME_DRIVER \
>>   -o 
>> .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi
>>  \
>>   
>> Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
> 
> Backtrace from gdb:
> 
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
>> Missing separate debuginfos, use: debuginfo-install 
>> glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
>> (gdb) where
>> #0  0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
>> #1  0x00407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", 
>> ZeroDebugFlag=0 '\000') at GenFw.c:2898
>> #2  0x00406a4d in main (argc=0, argv=0x7fffd0b8) at GenFw.c:2591
> 
> See below for the SIGSEGV location:
> 
> On 07/03/17 07:21, Liming Gao wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=600
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Liming Gao 
>> ---
>>  BaseTools/Source/C/GenFw/GenFw.c | 27 ++-
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/GenFw/GenFw.c 
>> b/BaseTools/Source/C/GenFw/GenFw.c
>> index 22e4e72..6569460 100644
>> --- a/BaseTools/Source/C/GenFw/GenFw.c
>> +++ b/BaseTools/Source/C/GenFw/GenFw.c
>> @@ -2770,6 +2770,7 @@ Returns:
>>  {
>>UINT32   Index;
>>UINT32   DebugDirectoryEntryRva;
>> +  UINT32   DebugDirectoryEntrySize;
>>UINT32   DebugDirectoryEntryFileOffset;
>>UINT32   ExportDirectoryEntryRva;
>>UINT32   ExportDirectoryEntryFileOffset;
>> @@ -2781,12 +2782,14 @@ Returns:
>>EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
>>EFI_IMAGE_SECTION_HEADER*SectionHeader;
>>EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
>> +  EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
>>UINT32  *NewTimeStamp;
>>
>>//
>>// Init variable.
>>//
>>DebugDirectoryEntryRva   = 0;
>> +  DebugDirectoryEntrySize  = 0;
>>ExportDirectoryEntryRva  = 0;
>>ResourceDirectoryEntryRva= 0;
>>DebugDirectoryEntryFileOffset= 0;
>> @@ -2822,6 +2825,7 @@ Returns:
>>  if (Optional32Hdr->NumberOfRvaAndSizes > 
>> EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>>  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
>> != 0) {
>>DebugDirectoryEntryRva = 
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> +  DebugDirectoryEntrySize = 
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>>if (ZeroDebugFlag) {
>>  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
>> = 0;
>>  
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress 
>> = 0;
>> @@ -2841,6 +2845,7 @@ Returns:
>>  if (Optional64Hdr->NumberOfRvaAndSizes > 
>> EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>>  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
>> != 0) {
>>DebugDirectoryEntryRva = 
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> +  DebugDirectoryEntrySize = 
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>>if (ZeroDebugFlag) {
>>  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
>> = 0;
>>  
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress 
>> = 0;
>> @@ -2886,11 +2891,23 @@ Returns:
>>
>>if (DebugDirectoryEntryFileOffset != 0) {
>>  DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
>> DebugDirectoryEntryFileOffset);
>> -DebugEntry->TimeDateStamp = 0;
>> -mImageTimeStamp = 0;
>> -if (ZeroDebugFlag) {
>> -  memset (FileBuffer + DebugEntry->FileOffset, 0, 
>> DebugEntry->SizeOfData);
>> -  memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>> +Index = 0;
>> +for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
>> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
>> +  DebugEntry->TimeDateStamp = 0;
>> +  if (ZeroDebugFlag || DebugEntry->Type != 
>> EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>> +memset (FileBuffer + DebugEntry->FileOffset, 0, 
>> DebugEntry->SizeOfData);
> 
> This memset() is the culprit.
> 
> According to gdb (all values decimal),
> - Index = 1,
> - DebugDirectoryEntrySize = 237,
> - ZeroDebugFlag 

[edk2] [PATCH] ArmVirtPkg: remove status code support

[Ard Biesheuvel]

Commit 7b1dc6c569a 'ArmVirtPkg: switch to generic ResetSystemRuntimeDxe'
replaced all references in ArmVirtPkg to the deprecated ResetRuntimeDxe
from EmbeddedPkg with the well maintained generic alternative that lives
in MdeModulePkg.

However, as it turns out, the generic driver has a dependency on the
library class ReportStatusCodeLib, whose default resolution is an
implementation that is not safe for use at runtime, resulting in crashes
when trying to invoke it from the OS.

Since we have no use for status codes in any of the ArmVirtPkg
platforms, let's replace all resolutions with a common one to the NULL
implementation.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmVirtPkg/ArmVirt.dsc.inc | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 0b9b457b5619..bfc40286d7ab 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -154,6 +154,8 @@ [LibraryClasses.common]
   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
   
UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
 
+  
ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
+
 [LibraryClasses.common.SEC]
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
@@ -174,7 +176,6 @@ [LibraryClasses.common.PEI_CORE]
   
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
   PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
   PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
-  
ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
   
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   
PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
   
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
@@ -191,7 +192,6 @@ [LibraryClasses.common.PEIM]
   
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
   PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
   PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
-  
ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
   
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   
PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
   
PeiResourcePublicationLib|MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.inf
@@ -205,13 +205,11 @@ [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
   
MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   
ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
   
PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
 
 [LibraryClasses.common.DXE_DRIVER]
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   
SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -223,10 +221,8 @@ [LibraryClasses.common.UEFI_APPLICATION]
   HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
-  
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
 
 [LibraryClasses.common.UEFI_DRIVER]
-  
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
   
UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf
   
ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
   PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
@@ -234,7 +230,6 @@ [LibraryClasses.common.UEFI_DRIVER]
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
-  

Re: [edk2] writing EDK compatible application.

[Rod Smith]

On 07/04/2017 04:10 PM, Laszlo Ersek wrote:
> On 07/04/17 19:20, Amit kumar wrote:
>>
>> HI,
>>
>> I have written a code (say helloworld program ) using edk2 framework, named 
>> the output efi file as BOOTx64.efi and placed it on a removable media in 
>> EFI/BOOT/ directory so that the application is listed in one time boot menu. 
>> When selected from boot menu it prints "Hello World".
>> Which works as expected when tested on UEFI 2.3 and above platforms.
>> But the same code fails to execute on EFI 1.10 platforms. Which i suppose is 
>> the problem with application entry point.
>> Can somebody suggest me a way so that the application entry function can be 
>> compatible to both the platform. Even when written according to UEFI 2.5+ 
>> Spec ?
> 
> I don't think you can develop for EFI 1 using edk2 -- unless you use
> EdkCompatibilityPkg I guess. But, I don't know how much
> EdkCompatibilityPkg is maintained.

Actually, I think it is possible -- or at the very least, it's possible
to write something that works on both UEFI-based PCs and EFI 1.1-based
Macs. My rEFInd (https://sourceforge.net/projects/refind/) does this.
I've used it with Macs as early as a first-generation 32-bit Mac Mini. I
have NOT tested it with any non-Apple EFI 1.x implementations, though. I
recall I had to fiddle with the .inf file to get it to work with Macs.
Comparing my refind.inf to other .inf files included in EDK2, I think
the relevant line(s) were one or both of the following:

  EDK_RELEASE_VERSION   = 0x0002
  EFI_SPECIFICATION_VERSION = 0x0001

This is in the [Defines] section.

Note that, if you want to look more closely at the rEFInd code or
configuration files, I've made some significant improvements to the
compilation procedures under EDK2 (vs. GNU-EFI) since the latest 0.10.8
release. Thus, you should pull down the latest unreleased code via git,
rather than download the source code tarball.

-- 
Rod Smith
rodsm...@rodsbooks.com
http://www.rodsbooks.com
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

[Laszlo Ersek]

Hi Liming,

Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.

I can reproduce the segfault locally. This is the command line:

> GenFw \
>   -e DXE_RUNTIME_DRIVER \
>   -o 
> .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi
>  \
>   
> Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll

Backtrace from gdb:

> Program received signal SIGSEGV, Segmentation fault.
> 0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
> Missing separate debuginfos, use: debuginfo-install glibc-2.17-194.el7.x86_64 
> libuuid-2.23.2-39.el7.x86_64
> (gdb) where
> #0  0x7789c814 in __memset_sse2 () from /lib64/libc.so.6
> #1  0x00407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", 
> ZeroDebugFlag=0 '\000') at GenFw.c:2898
> #2  0x00406a4d in main (argc=0, argv=0x7fffd0b8) at GenFw.c:2591

See below for the SIGSEGV location:

On 07/03/17 07:21, Liming Gao wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=600
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Liming Gao 
> ---
>  BaseTools/Source/C/GenFw/GenFw.c | 27 ++-
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c 
> b/BaseTools/Source/C/GenFw/GenFw.c
> index 22e4e72..6569460 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -2770,6 +2770,7 @@ Returns:
>  {
>UINT32   Index;
>UINT32   DebugDirectoryEntryRva;
> +  UINT32   DebugDirectoryEntrySize;
>UINT32   DebugDirectoryEntryFileOffset;
>UINT32   ExportDirectoryEntryRva;
>UINT32   ExportDirectoryEntryFileOffset;
> @@ -2781,12 +2782,14 @@ Returns:
>EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
>EFI_IMAGE_SECTION_HEADER*SectionHeader;
>EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
> +  EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
>UINT32  *NewTimeStamp;
>
>//
>// Init variable.
>//
>DebugDirectoryEntryRva   = 0;
> +  DebugDirectoryEntrySize  = 0;
>ExportDirectoryEntryRva  = 0;
>ResourceDirectoryEntryRva= 0;
>DebugDirectoryEntryFileOffset= 0;
> @@ -2822,6 +2825,7 @@ Returns:
>  if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG 
> && \
>  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> != 0) {
>DebugDirectoryEntryRva = 
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> +  DebugDirectoryEntrySize = 
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>if (ZeroDebugFlag) {
>  Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 
> 0;
>  
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress 
> = 0;
> @@ -2841,6 +2845,7 @@ Returns:
>  if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG 
> && \
>  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size 
> != 0) {
>DebugDirectoryEntryRva = 
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> +  DebugDirectoryEntrySize = 
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>if (ZeroDebugFlag) {
>  Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 
> 0;
>  
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress 
> = 0;
> @@ -2886,11 +2891,23 @@ Returns:
>
>if (DebugDirectoryEntryFileOffset != 0) {
>  DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + 
> DebugDirectoryEntryFileOffset);
> -DebugEntry->TimeDateStamp = 0;
> -mImageTimeStamp = 0;
> -if (ZeroDebugFlag) {
> -  memset (FileBuffer + DebugEntry->FileOffset, 0, 
> DebugEntry->SizeOfData);
> -  memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> +Index = 0;
> +for (Index=0; Index < DebugDirectoryEntrySize / sizeof 
> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
> +  DebugEntry->TimeDateStamp = 0;
> +  if (ZeroDebugFlag || DebugEntry->Type != 
> EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> +memset (FileBuffer + DebugEntry->FileOffset, 0, 
> DebugEntry->SizeOfData);

This memset() is the culprit.

According to gdb (all values decimal),
- Index = 1,
- DebugDirectoryEntrySize = 237,
- ZeroDebugFlag = 0.

These values look suspicious, because
sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
DebugDirectoryEntrySize (237) is not an integral multiple of that.

This is the first 

Re: [edk2] writing EDK compatible application.

[Amit kumar]

Laszlo, Andrew, Marvin


Thank you very much for your suggestions and for sharing information.

It really did help a lot.

Thanks and Regards

Amit


From: Laszlo Ersek 
Sent: Wednesday, July 5, 2017 1:40:49 AM
To: Amit kumar; edk2-devel@lists.01.org
Subject: Re: [edk2] writing EDK compatible application.

On 07/04/17 19:20, Amit kumar wrote:
>
> HI,
>
> I have written a code (say helloworld program ) using edk2 framework, named 
> the output efi file as BOOTx64.efi and placed it on a removable media in 
> EFI/BOOT/ directory so that the application is listed in one time boot menu. 
> When selected from boot menu it prints "Hello World".
> Which works as expected when tested on UEFI 2.3 and above platforms.
> But the same code fails to execute on EFI 1.10 platforms. Which i suppose is 
> the problem with application entry point.
> Can somebody suggest me a way so that the application entry function can be 
> compatible to both the platform. Even when written according to UEFI 2.5+ 
> Spec ?

I don't think you can develop for EFI 1 using edk2 -- unless you use
EdkCompatibilityPkg I guess. But, I don't know how much
EdkCompatibilityPkg is maintained.

Here's some links:

https://github.com/tianocore/tianocore.github.io/wiki/EdkCompatibilityPkg

https://github.com/tianocore/tianocore.github.io/wiki/EDK

The entry point is likely the least of your problems. The library
instances pulled in from under MdePkg, MdeModulePkg etc are full of UEFI
2.* dependencies, I'm pretty sure. I would suggest ignoring EFI 1...

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

Re: [edk2] [PATCH v2] ArmPlatformPkg: convert VExpress ResetSystemLib to ResetSystemLib

[Ryan Harkin]

On 4 July 2017 at 18:27, Ard Biesheuvel  wrote:
> On 4 July 2017 at 18:11, Leif Lindholm  wrote:
>> Since we're in the process of migrating all of the VExpress platforms
>> to MdeModulePkg ResetSystemRuntimeDxe, convert VExpress ResetSystemLib
>> from EfiResetSystemLib interface to the ResetSystemLib one.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Leif Lindholm 
>
> Reviewed-by: Ard Biesheuvel 

After popping commit e4129b0e5897d76885170bec9da996b266f185f9 off the
top of edk2, and adding this, I also applied patch "Platforms/ARM:
move ARM platforms to generic ResetSystemRuntimeDxe" to
OpenPlatformPkg and tested on TC2, FVP Foundation and AEMv8 models and
Juno R0/1/2.

I checked that reset and shutdown both still work as expected. Reset
was triggered from BDS, grub and EFI Shell. Shutdown (reset -s) was
trigger from EFI Shell.

Tested-by: Ryan Harkin 

>
>> ---
>>  .../Library/ResetSystemLib/ResetSystemLib.c| 117 
>> -
>>  .../Library/ResetSystemLib/ResetSystemLib.inf  |   6 +-
>>  2 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git 
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c 
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
>> index bafb6f8093..d2bc4a88fa 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
>> @@ -5,6 +5,7 @@
>>
>>Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
>>Copyright (c) 2013, ARM Ltd. All rights reserved.
>> +  Copyright (c) 2017, Linaro Ltd. All rights reserved.
>>
>>This program and the accompanying materials
>>are licensed and made available under the terms and conditions of the BSD 
>> License
>> @@ -16,73 +17,95 @@
>>
>>  **/
>>
>> -#include 
>> +#include 
>>
>> -#include 
>> -#include 
>> -#include 
>>  #include 
>> +#include 
>> +#include 
>>
>>  #include 
>>
>>  /**
>> -  Resets the entire platform.
>> +  This function causes a system-wide reset (cold reset), in which
>> +  all circuitry within the system returns to its initial state. This type of
>> +  reset is asynchronous to system operation and operates without regard to
>> +  cycle boundaries.
>>
>> -  @param  ResetType The type of reset to perform.
>> -  @param  ResetStatus   The status code for the reset.
>> -  @param  DataSize  The size, in bytes, of WatchdogData.
>> -  @param  ResetData For a ResetType of EfiResetCold, 
>> EfiResetWarm, or
>> -EfiResetShutdown the data buffer starts 
>> with a Null-terminated
>> -Unicode string, optionally followed by 
>> additional binary data.
>> +  If this function returns, it means that the system does not support cold
>> +  reset.
>> +**/
>> +VOID
>> +EFIAPI
>> +ResetCold (
>> +  VOID
>> +  )
>> +{
>> +  ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
>> +}
>> +
>> +/**
>> +  This function causes a system-wide initialization (warm reset), in which 
>> all
>> +  processors are set to their initial state. Pending cycles are not 
>> corrupted.
>>
>> +  If this function returns, it means that the system does not support warm
>> +  reset.
>>  **/
>> -EFI_STATUS
>> +VOID
>>  EFIAPI
>> -LibResetSystem (
>> -  IN EFI_RESET_TYPE   ResetType,
>> -  IN EFI_STATUS   ResetStatus,
>> -  IN UINTNDataSize,
>> -  IN CHAR16   *ResetData OPTIONAL
>> +ResetWarm (
>> +  VOID
>>)
>>  {
>> -  switch (ResetType) {
>> -  case EfiResetPlatformSpecific:
>> -// Map the platform specific reset as reboot
>> -  case EfiResetWarm:
>> -// Map a warm reset into a cold reset
>> -  case EfiResetCold:
>> -// Send the REBOOT function to the platform microcontroller
>> -ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
>> -
>> -// We should never be here
>> -while(1);
>> -  case EfiResetShutdown:
>> -// Send the SHUTDOWN function to the platform microcontroller
>> -ArmPlatformSysConfigSet (SYS_CFG_SHUTDOWN, 0);
>> -
>> -// We should never be here
>> -while(1);
>> -  }
>> -
>> -  ASSERT(FALSE);
>> -  return EFI_UNSUPPORTED;
>> +  ResetCold ();
>>  }
>>
>>  /**
>> -  Initialize any infrastructure required for LibResetSystem () to function.
>> +  This function causes the system to enter a power state equivalent
>> +  to the ACPI G2/S5 or G3 states.
>> +
>> +  If this function returns, it means that the system does not support shut 
>> down reset.
>> +**/
>> +VOID
>> +EFIAPI
>> +ResetShutdown (
>> +  VOID
>> +  )
>> +{
>> +  ArmPlatformSysConfigSet (SYS_CFG_SHUTDOWN, 0);
>> +}
>>
>> -  @param  ImageHandle   The firmware allocated handle for the EFI image.
>> -  @param  SystemTable   A pointer to the EFI System Table.
>> +/**
>> +  This function 

[edk2] SCT test on EFI_BIS_PROTOCOL

[Santhapur Naveen]

Hi experts,

For one of the SCT test, EFI_BIS_PROTOCOL was needed. But I've tried to find 
out which driver is installing this protocol.
But I find no instance of this.
Please provide your comments on this.

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

[edk2] [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory

[Ruiyu Ni]

The patch fixes two kinds of issues in DxeCore that accesses memory
which might be freed or owned by other modules.
The existing bugs don't cause functionality issue.

1. CoreValidateHandle() checks whether the handle is valid by
   validating its signature. The proper way is to check whether
   the handle is in the handle database.
2. CoreDisconnectControllersUsingProtocolInterface() and
   CoreOpenProtocol() de-reference Link pointer which is
   already freed. The proper way is to not de-reference the pointer.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Star Zeng 
Cc: Hao A Wu 
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 54 ++---
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b89148c8..8e2f361805 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -72,15 +72,20 @@ CoreValidateHandle (
   )
 {
   IHANDLE *Handle;
+  LIST_ENTRY  *Link;
 
-  Handle = (IHANDLE *)UserHandle;
-  if (Handle == NULL) {
+  if (UserHandle == NULL) {
 return EFI_INVALID_PARAMETER;
   }
-  if (Handle->Signature != EFI_HANDLE_SIGNATURE) {
-return EFI_INVALID_PARAMETER;
+
+  for (Link = gHandleList.ForwardLink; Link !=  Link = 
Link->ForwardLink) {
+Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
+if (Handle == (IHANDLE *) UserHandle) {
+  return EFI_SUCCESS;
+}
   }
-  return EFI_SUCCESS;
+
+  return EFI_INVALID_PARAMETER;
 }
 
 
@@ -643,19 +648,16 @@ CoreDisconnectControllersUsingProtocolInterface (
   //
   do {
 ItemFound = FALSE;
-for ( Link = Prot->OpenList.ForwardLink;
-  (Link != >OpenList) && !ItemFound;
-  Link = Link->ForwardLink ) {
+for (Link = Prot->OpenList.ForwardLink; Link != >OpenList; Link = 
Link->ForwardLink) {
   OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, 
OPEN_PROTOCOL_DATA_SIGNATURE);
   if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
-ItemFound = TRUE;
 CoreReleaseProtocolLock ();
 Status = CoreDisconnectController (UserHandle, OpenData->AgentHandle, 
NULL);
 CoreAcquireProtocolLock ();
-if (EFI_ERROR (Status)) {
-   ItemFound = FALSE;
-   break;
+if (!EFI_ERROR (Status)) {
+  ItemFound = TRUE;
 }
+break;
   }
 }
   } while (ItemFound);
@@ -664,21 +666,17 @@ CoreDisconnectControllersUsingProtocolInterface (
 //
 // Attempt to remove BY_HANDLE_PROTOOCL and GET_PROTOCOL and TEST_PROTOCOL 
Open List items
 //
-do {
-  ItemFound = FALSE;
-  for ( Link = Prot->OpenList.ForwardLink;
-(Link != >OpenList) && !ItemFound;
-Link = Link->ForwardLink ) {
-OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, 
OPEN_PROTOCOL_DATA_SIGNATURE);
-if ((OpenData->Attributes &
-(EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | 
EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
-  ItemFound = TRUE;
-  RemoveEntryList (>Link);
-  Prot->OpenListCount--;
-  CoreFreePool (OpenData);
-}
+for (Link = Prot->OpenList.ForwardLink; Link != >OpenList;) {
+  OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, 
OPEN_PROTOCOL_DATA_SIGNATURE);
+  if ((OpenData->Attributes &
+  (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | 
EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
+Link = RemoveEntryList (>Link);
+Prot->OpenListCount--;
+CoreFreePool (OpenData);
+  } else {
+Link = Link->ForwardLink;
   }
-} while (ItemFound);
+}
   }
 
   //
@@ -1132,7 +1130,7 @@ CoreOpenProtocol (
 if (ByDriver) {
   do {
 Disconnect = FALSE;
-for ( Link = Prot->OpenList.ForwardLink; (Link != >OpenList) && 
(!Disconnect); Link = Link->ForwardLink) {
+for (Link = Prot->OpenList.ForwardLink; Link != >OpenList; Link 
= Link->ForwardLink) {
   OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, 
OPEN_PROTOCOL_DATA_SIGNATURE);
   if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
 Disconnect = TRUE;
@@ -1142,6 +1140,8 @@ CoreOpenProtocol (
 if (EFI_ERROR (Status)) {
   Status = EFI_ACCESS_DENIED;
   goto Done;
+} else {
+  break;
 }
   }
 }
-- 
2.12.2.windows.2

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

[edk2] [PATCH v2 1/2] EmbeddedPkg/DwEmmcDxe: limit max clock for platform

[Jun Nie]

Some boards may have max clock limitation. Add a Pcd to notify
driver.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie 
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c   | 4 
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 +
 EmbeddedPkg/EmbeddedPkg.dec | 1 +
 3 files changed, 6 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c 
b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index fe23d11..bb26b69 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -560,6 +560,10 @@ DwEmmcSetIos (
   EFI_STATUS Status = EFI_SUCCESS;
   UINT32Data;
 
+  if ((PcdGet32 (PcdDwEmmcDxeMaxClockFreqInHz) != 0) &&
+  (BusClockFreq > PcdGet32 (PcdDwEmmcDxeMaxClockFreqInHz))) {
+return EFI_UNSUPPORTED;
+  }
   if (TimingMode != EMMCBACKWARD) {
 Data = MmioRead32 (DWEMMC_UHSREG);
 switch (TimingMode) {
diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf 
b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
index e3c8313..99b4f99 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
@@ -48,6 +48,7 @@
 [Pcd]
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
+  gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz
 
 [Depex]
   TRUE
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 5ea2f22..2da9b2f 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -169,6 +169,7 @@
   # DwEmmc Driver PCDs
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x0035
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x0036
+  gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz|0x0|UINT32|0x0037
 
   #
   # Android FastBoot
-- 
1.9.1

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

[edk2] [PATCH v2 2/2] EmbeddedPkg/DwEmmc: Adjust FIFO threshold

[Jun Nie]

Adjust FIFO threshold according to FIFO depth. Skip
the adjustment if we do not have FIFO depth info.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie 
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h  |  6 
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c   | 50 +
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf |  1 +
 EmbeddedPkg/EmbeddedPkg.dec |  1 +
 4 files changed, 58 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h 
b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
index 055f1e0..90c7676 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
@@ -38,7 +38,10 @@
 #define DWEMMC_RINTSTS  ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x044)
 #define DWEMMC_STATUS   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x048)
 #define DWEMMC_FIFOTH   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x04c)
+#define DWEMMC_TCBCNT   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x05c)
+#define DWEMMC_TBBCNT   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x060)
 #define DWEMMC_DEBNCE   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x064)
+#define DWEMMC_HCON ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x070)
 #define DWEMMC_UHSREG   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x074)
 #define DWEMMC_BMOD ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x080)
 #define DWEMMC_DBADDR   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x088)
@@ -47,6 +50,7 @@
 #define DWEMMC_DSCADDR  ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x094)
 #define DWEMMC_BUFADDR  ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0x098)
 #define DWEMMC_CARDTHRCTL   ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0X100)
+#define DWEMMC_DATA ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress) + 
0X200)
 
 #define CMD_UPDATE_CLK  0x80202000
 #define CMD_START_BIT   (1 << 31)
@@ -124,4 +128,6 @@
 #define DWEMMC_CARD_RD_THR(x)   ((x & 0xfff) << 16)
 #define DWEMMC_CARD_RD_THR_EN   (1 << 0)
 
+#define DWEMMC_GET_HDATA_WIDTH(x)   (((x) >> 7) & 0x7)
+
 #endif  // __DWEMMC_H__
diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c 
b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index bb26b69..70e064d 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -415,6 +415,55 @@ DwEmmcReceiveResponse (
   return EFI_SUCCESS;
 }
 
+VOID DwEmmcAdjustFifothreshold (
+  VOID
+  )
+{
+  /* DMA multiple transaction size map to reg value as array index */
+  CONST UINT32 BurstSize[] = {1, 4, 8, 16, 32, 64, 128, 256};
+  UINT32 BlkDepthInFifo, Fifoth, FifoWidth, FifoDepth;
+  UINT32 BlkSize = DWEMMC_BLOCK_SIZE, Idx = 0, RxWmark = 1, TxWmark, 
TxWmarkInvers;
+
+  /* Skip FIFO adjustment if we do not have platform FIFO depth info */
+  FifoDepth = PcdGet32 (PcdDwEmmcDxeFifoDepth);
+  if (!FifoDepth) {
+return;
+  }
+
+  TxWmark = FifoDepth / 2;
+  TxWmarkInvers = FifoDepth - TxWmark;
+
+  FifoWidth = DWEMMC_GET_HDATA_WIDTH (MmioRead32 (DWEMMC_HCON));
+  if (!FifoWidth) {
+FifoWidth = 2;
+  } else if (FifoWidth == 2) {
+FifoWidth = 8;
+  } else {
+FifoWidth = 4;
+  }
+
+  BlkDepthInFifo = BlkSize / FifoWidth;
+
+  /* if BlkSize is not a multiple of the FIFO width */
+  if (BlkSize % FifoWidth) {
+goto done;
+  }
+
+  Idx = ARRAY_SIZE (BurstSize) - 1;
+  while (Idx && ((BlkDepthInFifo % BurstSize[Idx]) || (TxWmarkInvers % 
BurstSize[Idx]))) {
+Idx--;
+  }
+  RxWmark = BurstSize[Idx] - 1;
+  /*
+   * If Idx is '0', it won't be tried
+   * Thus, initial values are used
+   */
+done:
+  Fifoth = DWEMMC_DMA_BURST_SIZE (Idx) | DWEMMC_FIFO_TWMARK (TxWmark)
+   | DWEMMC_FIFO_RWMARK (RxWmark);
+  MmioWrite32 (DWEMMC_FIFOTH, Fifoth);
+}
+
 EFI_STATUS
 PrepareDmaData (
   IN DWEMMC_IDMAC_DESCRIPTOR*IdmacDesc,
@@ -633,6 +682,7 @@ DwEmmcDxeInitialize (
 
   Handle = NULL;
 
+  DwEmmcAdjustFifothreshold ();
   gpIdmacDesc = (DWEMMC_IDMAC_DESCRIPTOR *)AllocatePages 
(DWEMMC_MAX_DESC_PAGES);
   if (gpIdmacDesc == NULL) {
 return EFI_BUFFER_TOO_SMALL;
diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf 
b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
index 99b4f99..bc4413e 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
@@ -49,6 +49,7 @@
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
   gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz
+  gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeFifoDepth
 
 [Depex]
   TRUE
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 2da9b2f..5f39d9d 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -170,6 +170,7 @@
   

Re: [edk2] [PATCH 0/3] Disk Information Protocol for SD/MMC devices

[Zeng, Star]

Reviewed-by: Star Zeng  to the patch series.

-Original Message-
From: Wu, Hao A 
Sent: Wednesday, July 5, 2017 1:05 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Ni, Ruiyu ; Zeng, Star 

Subject: [PATCH 0/3] Disk Information Protocol for SD/MMC devices

Per PI 1.6 spec, add Disk Information Protocol support in SD/MMC devices.

Cc: Ruiyu Ni 
Cc: Star Zeng 

Hao Wu (3):
  MdePkg/DiskInfo.h: Add the SD/MMC interface GUID definition
  MdeModulePkg/SdDxe: Implementation of Disk Information Protocol
  MdeModulePkg/EmmcDxe: Implementation of Disk Information Protocol

 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDiskInfo.c | 140 + 
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDiskInfo.h | 115 
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c  |  15 +++-
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.h  |   9 +-
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf|   5 +-
 MdeModulePkg/Bus/Sd/SdDxe/SdDiskInfo.c | 138 
 MdeModulePkg/Bus/Sd/SdDxe/SdDiskInfo.h | 115 
 MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  13 ++-
 MdeModulePkg/Bus/Sd/SdDxe/SdDxe.h  |   9 +-
 MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf|   5 +-
 MdePkg/Include/Protocol/DiskInfo.h |  13 ++-
 MdePkg/MdePkg.dec  |   6 ++
 12 files changed, 575 insertions(+), 8 deletions(-)  create mode 100644 
MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDiskInfo.c
 create mode 100644 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDiskInfo.h
 create mode 100644 MdeModulePkg/Bus/Sd/SdDxe/SdDiskInfo.c
 create mode 100644 MdeModulePkg/Bus/Sd/SdDxe/SdDiskInfo.h

--
2.12.0.windows.1

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

Re: [edk2] [Patch] MdePkg: Declare _ReturnAddress() in Base.h for MSFT tool chain

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Liming 
Gao
Sent: Wednesday, July 5, 2017 1:57 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D 
Subject: [edk2] [Patch] MdePkg: Declare _ReturnAddress() in Base.h for MSFT 
tool chain

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

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao 
Cc: Michael Kinney 
---
 MdePkg/Include/Base.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index 
21a603a..02140a5 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -1213,6 +1213,7 @@ typedef UINTN RETURN_STATUS;
 (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << 32))
 
 #if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && !defined 
(MDE_CPU_EBC)
+  void * _ReturnAddress(void);
   #pragma intrinsic(_ReturnAddress)
   /**
 Get the return address of the calling function.
--
2.8.0.windows.1

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