Re: [edk2] [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs

[Wang, Jian J]

Tests:
a. try all related PCDs combinations and check the page table attributes
and ASSERT message
b. boot to shell on real intel platform with valid PCD setting combinations 
(IA32/X64)
c. boot to fedora26, ubuntu18.04, windows 7 and windows 10 on OVMF  emulated
platform (X64)

Regards,
Jian

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> Sent: Friday, September 14, 2018 1:14 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Yao, Jiewen ;
> Laszlo Ersek ; Zeng, Star 
> Subject: [edk2] [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs
> 
> BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
> 
> Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This
> confuses developers because following two other PCDs also need NXE
> to be set, but actually not.
> 
> PcdDxeNxMemoryProtectionPolicy
> PcdImageProtectionPolicy
> 
> This patch solves this issue by adding logic to enable IA32_EFER.NXE
> if any of those PCDs have anything enabled.
> 
> Due to the fact that NX memory type of stack (enabled by PcdSetNxForStack)
> and image data section (enabled by PcdImageProtectionPolicy) are also
> part of PcdDxeNxMemoryProtectionPolicy, this patch also add more checks
> to warn (ASSERT) users any unreasonable setting combinations. For example,
> 
> PcdSetNxForStack == FALSE &&
>   (PcdDxeNxMemoryProtectionPolicy & (1 < 
> PcdImageProtectionPolicy == 0 &&
>   (PcdDxeNxMemoryProtectionPolicy & (1 << EfiRuntimeServicesData)) != 0
> 
> PcdImageProtectionPolicy == 0 &&
>   (PcdDxeNxMemoryProtectionPolicy & (1 < 
> PcdImageProtectionPolicy == 0 &&
>   (PcdDxeNxMemoryProtectionPolicy & (1 < 
> In other words, PcdSetNxForStack and PcdImageProtectionPolicy have
> priority over PcdDxeNxMemoryProtectionPolicy.
> 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Cc: Ruiyu Ni 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  2 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  4 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 55
> +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 33 ++
>  4 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index fd82657404..068e700074 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -117,6 +117,8 @@
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack   ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ##
> SOMETIMES_CONSUMES
> 
>  [Depex]
>gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index d28baa3615..9a97205ef8 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -245,7 +245,7 @@ ToBuildPageTable (
>  return TRUE;
>}
> 
> -  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable ()) {
> +  if (ToEnableExecuteDisableFeature ()) {
>  return TRUE;
>}
> 
> @@ -436,7 +436,7 @@ HandOffToDxeCore (
>  BuildPageTablesIa32Pae = ToBuildPageTable ();
>  if (BuildPageTablesIa32Pae) {
>PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
> -  if (IsExecuteDisableBitAvailable ()) {
> +  if (ToEnableExecuteDisableFeature ()) {
>  EnableExecuteDisableBit();
>}
>  }
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 496e219913..253fe84223 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -106,6 +106,56 @@ IsNullDetectionEnabled (
>return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0);
>  }
> 
> +/**
> +  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not.
> +
> +  @retval TRUEIA32_EFER.NXE should be enabled.
> +  @retval FALSE   IA32_EFER.NXE should not be enabled.
> +
> +**/
> +BOOLEAN
> +ToEnableExecuteDisableFeature (
> +  VOID
> +  )
> +{
> +  if (!IsExecuteDisableBitAvailable ()) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Normally stack is type of EfiBootServicesData. Disabling NX for stack
> +  // but enabling NX for EfiBootServicesData doesn't make any sense.
> +  //
> +  if (PcdGetBool (PcdSetNxForStack) == FALSE &&
> +  (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> STACK_MEMORY_TYPE) != 0) {
> +DEBUG ((DEBUG_ERROR,
> +"ERROR: NX for stack is disabled but NX for its 

[edk2] [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs

[Jian J Wang]

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

Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This
confuses developers because following two other PCDs also need NXE
to be set, but actually not.

PcdDxeNxMemoryProtectionPolicy
PcdImageProtectionPolicy

This patch solves this issue by adding logic to enable IA32_EFER.NXE
if any of those PCDs have anything enabled.

Due to the fact that NX memory type of stack (enabled by PcdSetNxForStack)
and image data section (enabled by PcdImageProtectionPolicy) are also
part of PcdDxeNxMemoryProtectionPolicy, this patch also add more checks
to warn (ASSERT) users any unreasonable setting combinations. For example,

PcdSetNxForStack == FALSE &&
  (PcdDxeNxMemoryProtectionPolicy & (1 <
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  2 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  4 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 55 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 33 ++
 4 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf 
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index fd82657404..068e700074 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -117,6 +117,8 @@
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack   ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
SOMETIMES_CONSUMES
 
 [Depex]
   gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c 
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index d28baa3615..9a97205ef8 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -245,7 +245,7 @@ ToBuildPageTable (
 return TRUE;
   }
 
-  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable ()) {
+  if (ToEnableExecuteDisableFeature ()) {
 return TRUE;
   }
 
@@ -436,7 +436,7 @@ HandOffToDxeCore (
 BuildPageTablesIa32Pae = ToBuildPageTable ();
 if (BuildPageTablesIa32Pae) {
   PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
-  if (IsExecuteDisableBitAvailable ()) {
+  if (ToEnableExecuteDisableFeature ()) {
 EnableExecuteDisableBit();
   }
 }
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c 
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 496e219913..253fe84223 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -106,6 +106,56 @@ IsNullDetectionEnabled (
   return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0);
 }
 
+/**
+  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not.
+
+  @retval TRUEIA32_EFER.NXE should be enabled.
+  @retval FALSE   IA32_EFER.NXE should not be enabled.
+
+**/
+BOOLEAN
+ToEnableExecuteDisableFeature (
+  VOID
+  )
+{
+  if (!IsExecuteDisableBitAvailable ()) {
+return FALSE;
+  }
+
+  //
+  // Normally stack is type of EfiBootServicesData. Disabling NX for stack
+  // but enabling NX for EfiBootServicesData doesn't make any sense.
+  //
+  if (PcdGetBool (PcdSetNxForStack) == FALSE &&
+  (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & STACK_MEMORY_TYPE) != 0) {
+DEBUG ((DEBUG_ERROR,
+"ERROR: NX for stack is disabled but NX for its memory type is 
enabled!\r\n"));
+ASSERT(!(PcdGetBool (PcdSetNxForStack) == FALSE &&
+ (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & STACK_MEMORY_TYPE) 
!= 0));
+  }
+
+  //
+  // Image data section could be type of EfiLoaderData, EfiBootServicesData
+  // or EfiRuntimeServicesData. Disabling NX for image data but enabling NX
+  // for any those memory types doesn't make any sense.
+  //
+  if (PcdGet32 (PcdImageProtectionPolicy) == 0 &&
+  (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & IMAGE_DATA_MEMORY_TYPE) != 
0) {
+DEBUG ((DEBUG_ERROR,
+"ERROR: NX for image data is disabled but NX for its memory 
type(s) is enabled!\r\n"));
+ASSERT (!(PcdGet32 (PcdImageProtectionPolicy) == 0 &&
+  (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & 
IMAGE_DATA_MEMORY_TYPE) != 0));
+  }
+
+  //
+  // XD flag (BIT63) in page table entry is only valid if IA32_EFER.NXE is set.
+  // Features controlled by Following PCDs need this feature to be enabled.
+  //
+  return (PcdGetBool (PcdSetNxForStack) ||
+  PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 ||
+  PcdGet32 (PcdImageProtectionPolicy) != 0);
+}
+
 /**
   Enable Execute Disable Bit.
 
@@ -755,7 +805,10 @@ 

Re: [edk2] [Patch] BaseTools: Fix a bug for Unused PCDs section display in the report

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu 

Best Regards,
Zhu Yonghong


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong 
Zhu
Sent: Thursday, September 13, 2018 1:36 PM
To: edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: [edk2] [Patch] BaseTools: Fix a bug for Unused PCDs section display in 
the report

From: zhijufan 

Fix a regression issue caused by ac4578af364, when there doesn't exist not used 
PCD, it also display the not used Pcd section in the report.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=1170
Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/build/BuildReport.py | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/build/BuildReport.py 
b/BaseTools/Source/Python/build/BuildReport.py
index c7fa1b9..49bcd9c 100644
--- a/BaseTools/Source/Python/build/BuildReport.py
+++ b/BaseTools/Source/Python/build/BuildReport.py
@@ -886,11 +886,21 @@ class PcdReport(object):
 def GenerateReport(self, File, ModulePcdSet):
 if not ModulePcdSet:
 if self.ConditionalPcds:
 self.GenerateReportDetail(File, ModulePcdSet, 1)
 if self.UnusedPcds:
-self.GenerateReportDetail(File, ModulePcdSet, 2)
+IsEmpty = True
+for Token in self.UnusedPcds:
+TokenDict = self.UnusedPcds[Token]
+for Type in TokenDict:
+if TokenDict[Type]:
+IsEmpty = False
+break
+if not IsEmpty:
+break
+if not IsEmpty:
+self.GenerateReportDetail(File, ModulePcdSet, 2)
 self.GenerateReportDetail(File, ModulePcdSet)
 
 ##
 # Generate report for PCD information
 #
--
2.6.1.windows.1

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

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

[Ni, Ruiyu]

On 9/14/2018 3:31 AM, Duran, Leo wrote:




-Original Message-
From: Ni, Ruiyu 
Sent: Wednesday, September 12, 2018 9:39 PM
To: Duran, Leo ; Laszlo Ersek ;
edk2-devel@lists.01.org
Cc: Dong, Eric 
Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
prior to MTRR change.

Leo,
Sorry I was in leave yesterday so didn't see the mail.
Which MSRs are shared? Only the MSR_IA32_MTRR_DEF_TYPE_REGISTER?
Or all the MSRs that configures the CPU MTRR setting?



Hi Ray,
The MTTR config MSRs are also shared by threads within a core.



Hi Leo,
Do you think that fixing the caller is more proper?


I also agree with Laszlo's comments to fix the caller if all MSRs relating to
MTRR are shared.
That will be to fix MpInitLib and CpuDxe driver.

Thanks/Ray


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

Re: [edk2] [PATCH V2 4/6] PcAtChipsetPkg PcRtc: Use new EfiLocateFirstAcpiTable()

[Ni, Ruiyu]

On 9/14/2018 12:41 PM, Ni, Ruiyu wrote:

On 9/13/2018 6:26 PM, Star Zeng wrote:

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates PcatRealTimeClockRuntimeDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 80 
+-

  1 file changed, 3 insertions(+), 77 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c 
b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c

index 2105acf35f7b..7965eb8aa55b 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -1203,49 +1203,6 @@ IsWithinOneDay (
  }
  /**
-  This function find ACPI table with the specified signature in RSDT 
or XSDT.

-
-  @param Sdt  ACPI RSDT or XSDT.
-  @param Signature    ACPI table signature.
-  @param TablePointerSize Size of table pointer: 4 or 8.
-
-  @return ACPI table or NULL if not found.
-**/
-VOID *
-ScanTableInSDT (
-  IN EFI_ACPI_DESCRIPTION_HEADER    *Sdt,
-  IN UINT32 Signature,
-  IN UINTN  TablePointerSize
-  )
-{
-  UINTN  Index;
-  UINTN  EntryCount;
-  UINTN  EntryBase;
-  EFI_ACPI_DESCRIPTION_HEADER    *Table;
-
-  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
TablePointerSize;

-
-  EntryBase = (UINTN) (Sdt + 1);
-  for (Index = 0; Index < EntryCount; Index++) {
-    //
-    // When TablePointerSize is 4 while sizeof (VOID *) is 8, make 
sure the upper 4 bytes are zero.

-    //
-    Table = 0;
-    CopyMem (, (VOID *) (EntryBase + Index * TablePointerSize), 
TablePointerSize);

-
-    if (Table == NULL) {
-  continue;
-    }
-
-    if (Table->Signature == Signature) {
-  return Table;
-    }
-  }
-
-  return NULL;
-}
-
-/**
    Get the century RTC address from the ACPI FADT table.
    @return  The century RTC address or 0 if not found.
@@ -1255,42 +1212,11 @@ GetCenturyRtcAddress (
    VOID
    )
  {
-  EFI_STATUS    Status;
-  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
    EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
-  Status = EfiGetSystemConfigurationTable (, (VOID 
**) );

-  if (EFI_ERROR (Status)) {
-    Status = EfiGetSystemConfigurationTable (, 
(VOID **) );

-  }
-
-  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
-    return 0;
-  }
-
-  Fadt = NULL;
-
-  //
-  // Find FADT in XSDT
-  //
-  if (Rsdp->Revision >= 
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION && 
Rsdp->XsdtAddress != 0) {

-    Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINTN)
- );
-  }
-
-  //
-  // Find FADT in RSDT
-  //
-  if (Fadt == NULL && Rsdp->RsdtAddress != 0) {
-    Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINT32)
- );
-  }
+  Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) 
EfiLocateFirstAcpiTable (
+ 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE

+ );
    if ((Fadt != NULL) &&
    (Fadt->Century > RTC_ADDRESS_REGISTER_D) && (Fadt->Century < 
0x80)




Reviewed-by: Ruiyu Ni 


By the way, can we remove the Acpi10/Acpi20 GUID references from INF file?

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

Re: [edk2] [PATCH V2 4/6] PcAtChipsetPkg PcRtc: Use new EfiLocateFirstAcpiTable()

[Ni, Ruiyu]

On 9/13/2018 6:26 PM, Star Zeng wrote:

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates PcatRealTimeClockRuntimeDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 80 +-
  1 file changed, 3 insertions(+), 77 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c 
b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 2105acf35f7b..7965eb8aa55b 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -1203,49 +1203,6 @@ IsWithinOneDay (
  }
  
  /**

-  This function find ACPI table with the specified signature in RSDT or XSDT.
-
-  @param Sdt  ACPI RSDT or XSDT.
-  @param SignatureACPI table signature.
-  @param TablePointerSize Size of table pointer: 4 or 8.
-
-  @return ACPI table or NULL if not found.
-**/
-VOID *
-ScanTableInSDT (
-  IN EFI_ACPI_DESCRIPTION_HEADER*Sdt,
-  IN UINT32 Signature,
-  IN UINTN  TablePointerSize
-  )
-{
-  UINTN  Index;
-  UINTN  EntryCount;
-  UINTN  EntryBase;
-  EFI_ACPI_DESCRIPTION_HEADER*Table;
-
-  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
TablePointerSize;
-
-  EntryBase = (UINTN) (Sdt + 1);
-  for (Index = 0; Index < EntryCount; Index++) {
-//
-// When TablePointerSize is 4 while sizeof (VOID *) is 8, make sure the 
upper 4 bytes are zero.
-//
-Table = 0;
-CopyMem (, (VOID *) (EntryBase + Index * TablePointerSize), 
TablePointerSize);
-
-if (Table == NULL) {
-  continue;
-}
-
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
Get the century RTC address from the ACPI FADT table.
  
@return  The century RTC address or 0 if not found.

@@ -1255,42 +1212,11 @@ GetCenturyRtcAddress (
VOID
)
  {
-  EFI_STATUSStatus;
-  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
  
-  Status = EfiGetSystemConfigurationTable (, (VOID **) );

-  if (EFI_ERROR (Status)) {
-Status = EfiGetSystemConfigurationTable (, (VOID **) 
);
-  }
-
-  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
-return 0;
-  }
-
-  Fadt = NULL;
-
-  //
-  // Find FADT in XSDT
-  //
-  if (Rsdp->Revision >= EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION && 
Rsdp->XsdtAddress != 0) {
-Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINTN)
- );
-  }
-
-  //
-  // Find FADT in RSDT
-  //
-  if (Fadt == NULL && Rsdp->RsdtAddress != 0) {
-Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINT32)
- );
-  }
+  Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) EfiLocateFirstAcpiTable 
(
+ 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE
+ );
  
if ((Fadt != NULL) &&

(Fadt->Century > RTC_ADDRESS_REGISTER_D) && (Fadt->Century < 0x80)



Reviewed-by: Ruiyu Ni 

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

Re: [edk2] [PATCH V2 1/6] MdePkg UefiLib: Add new EfiLocateXXXAcpiTable() APIs

[Ni, Ruiyu]

Star,
I have two comments. see below.
On 9/13/2018 6:26 PM, Star Zeng wrote:

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch adds new EfiLocateXXXAcpiTable() APIs in UefiLib
for the request and also the following patch to remove the
duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
  MdePkg/Include/Library/UefiLib.h   |  68 ++
  MdePkg/Library/UefiLib/Acpi.c  | 488 +
  MdePkg/Library/UefiLib/UefiLib.inf |   3 +
  3 files changed, 559 insertions(+)
  create mode 100644 MdePkg/Library/UefiLib/Acpi.c

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index f80067f11103..cf82ff4a920a 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -26,6 +26,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
  #ifndef __UEFI_LIB_H__
  #define __UEFI_LIB_H__
  
+#include 

+
  #include 
  #include 
  #include 
@@ -1595,4 +1597,70 @@ EfiOpenFileByDevicePath (
IN UINT64OpenMode,
IN UINT64Attributes
);
+
+/**
+  This function locates next ACPI table in XSDT/RSDT based on Signature and
+  previous returned Table.
+
+  If PreviousTable is NULL:
+  This function will locate the first ACPI table in XSDT/RSDT based on
+  Signature in gEfiAcpi20TableGuid system configuration table first, and then
+  gEfiAcpi10TableGuid system configuration table.
+  This function will locate in XSDT first, and then RSDT.
+  For DSDT, this function will locate XDsdt in FADT first, and then Dsdt in
+  FADT.
+  For FACS, this function will locate XFirmwareCtrl in FADT first, and then
+  FirmwareCtrl in FADT.
+
+  If PreviousTable is not NULL:
+  1. If it could be located in XSDT in gEfiAcpi20TableGuid system configuration
+ table, then this function will just locate next table in XSDT in
+ gEfiAcpi20TableGuid system configuration table.
+  2. If it could be located in RSDT in gEfiAcpi20TableGuid system configuration
+ table, then this function will just locate next table in RSDT in
+ gEfiAcpi20TableGuid system configuration table.
+  3. If it could be located in RSDT in gEfiAcpi10TableGuid system configuration
+ table, then this function will just locate next table in RSDT in
+ gEfiAcpi10TableGuid system configuration table.
+


1. I don't see difference between the case when PreviousTable is NULL or 
not. We don't need to distinguish between the two cases in above comments.




+  If PreviousTable is not NULL and PreviousTable->Signature is not same with
+  Signature, then ASSERT.

I'd suggest to return Invalid Parameter instead of assertion.


+
+  @param Signature  ACPI table signature.
+  @param PreviousTable  Pointer to previous returned table to locate next
+table, or NULL to locate first table.
+
+  @return Next ACPI table or NULL if not found.
+
+**/
+EFI_ACPI_COMMON_HEADER *
+EFIAPI
+EfiLocateNextAcpiTable (
+  IN UINT32 Signature,
+  IN EFI_ACPI_COMMON_HEADER *PreviousTable OPTIONAL
+  );
+
+/**
+  This function locates first ACPI table in XSDT/RSDT based on Signature.
+
+  This function will locate the first ACPI table in XSDT/RSDT based on
+  Signature in gEfiAcpi20TableGuid system configuration table first, and then
+  gEfiAcpi10TableGuid system configuration table.
+  This function will locate in XSDT first, and then RSDT.
+  For DSDT, this function will locate XDsdt in FADT first, and then Dsdt in
+  FADT.
+  For FACS, this function will locate XFirmwareCtrl in FADT first, and then
+  FirmwareCtrl in FADT.
+
+  @param Signature  ACPI table signature.
+
+  @return First ACPI table or NULL if not found.
+
+**/
+EFI_ACPI_COMMON_HEADER *
+EFIAPI
+EfiLocateFirstAcpiTable (
+  IN UINT32 Signature
+  );
+
  #endif
diff --git a/MdePkg/Library/UefiLib/Acpi.c b/MdePkg/Library/UefiLib/Acpi.c
new file mode 100644
index ..0793bbdc787f
--- /dev/null
+++ b/MdePkg/Library/UefiLib/Acpi.c
@@ -0,0 +1,488 @@
+/** @file
+  This module provides help function for finding ACPI table.
+
+  Copyright (c) 2018, Intel Corporation. All rights reserved.
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php.
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+

[edk2] [PATCH] BaseTools: Regression bug Linux script used windows line format

[Feng, YunhuaX]

regression by 15e20228258c1714cd90207a52101a5b1b54cd2c
and 9f3594782de9051cbf599f9af006903ed3f6669e
Linux execute script must use '\n' not '\r\n' for end of line

Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yunhua Feng 
---
 BaseTools/BinWrappers/PosixLike/BPDG   | 2 +-
 BaseTools/BinWrappers/PosixLike/GenFds | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/BinWrappers/PosixLike/BPDG 
b/BaseTools/BinWrappers/PosixLike/BPDG
index bca1bae96a..276c7ea207 100755
--- a/BaseTools/BinWrappers/PosixLike/BPDG
+++ b/BaseTools/BinWrappers/PosixLike/BPDG
@@ -9,6 +9,6 @@ fi
 full_cmd=${BASH_SOURCE:-$0} # see http://mywiki.wooledge.org/BashFAQ/028 for a 
discussion of why $0 is not a good choice here
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
 export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
-exec "${python_exe:-python}" -m $cmd.$cmd "$@"
+exec "${python_exe:-python}" -m $cmd.$cmd "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/GenFds 
b/BaseTools/BinWrappers/PosixLike/GenFds
index bca1bae96a..276c7ea207 100755
--- a/BaseTools/BinWrappers/PosixLike/GenFds
+++ b/BaseTools/BinWrappers/PosixLike/GenFds
@@ -9,6 +9,6 @@ fi
 full_cmd=${BASH_SOURCE:-$0} # see http://mywiki.wooledge.org/BashFAQ/028 for a 
discussion of why $0 is not a good choice here
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
 export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
-exec "${python_exe:-python}" -m $cmd.$cmd "$@"
+exec "${python_exe:-python}" -m $cmd.$cmd "$@"
-- 
2.12.2.windows.2

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

Re: [edk2] [PATCH V2 6/6] UefiCpuPkg PiSmmCpuDxeSmm: Use new EfiLocateFirstAcpiTable()

[Dong, Eric]

Reviewed-by: Eric Dong  

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 13, 2018 6:27 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Younas khan
> ; Kinney, Michael D
> ; Gao, Liming ; Yao,
> Jiewen ; Ni, Ruiyu ; Dong, Eric
> ; Laszlo Ersek 
> Subject: [PATCH V2 6/6] UefiCpuPkg PiSmmCpuDxeSmm: Use new
> EfiLocateFirstAcpiTable()
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=967
> Request to add a library function for GetAcpiTable() in order to get ACPI 
> table
> using signature as input.
> 
> After evaluation, we found there are many duplicated code to find ACPI
> table by signature in different modules.
> 
> This patch updates PiSmmCpuDxeSmm to use new
> EfiLocateFirstAcpiTable() and remove the duplicated code.
> 
> Cc: Younas khan 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |  4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 84 ++--
> --
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |  3 +-
>  3 files changed, 6 insertions(+), 85 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index a7fb7b0b1482..0fdc1b134ea3 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -4,7 +4,7 @@
>  # This SMM driver performs SMM initialization, deploy SMM Entry Vector,  #
> provides CPU specific services in SMM.
>  #
> -# Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> +# Copyright (c) 2009 - 2018, Intel Corporation. All rights
> +reserved.
>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.  #  # This
> program and the accompanying materials @@ -114,8 +114,6 @@ [Protocols]
> [Guids]
>gEfiAcpiVariableGuid ## SOMETIMES_CONSUMES ## HOB # it 
> is
> used for S3 boot.
>gEfiGlobalVariableGuid   ## SOMETIMES_PRODUCES ##
> Variable:L"SmmProfileData"
> -  gEfiAcpi20TableGuid  ## SOMETIMES_CONSUMES ##
> SystemTable
> -  gEfiAcpi10TableGuid  ## SOMETIMES_CONSUMES ##
> SystemTable
>gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
>gEfiMemoryAttributesTableGuid## CONSUMES ## SystemTable
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index a743cf64f907..91b8e7ddb991 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Enable SMM profile.
> 
> -Copyright (c) 2012 - 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.
> 
>  This program and the accompanying materials @@ -726,84 +726,6 @@
> InitPaging (  }
> 
>  /**
> -  To find FADT in ACPI tables.
> -
> -  @param AcpiTableGuid   The GUID used to find ACPI table in UEFI
> ConfigurationTable.
> -
> -  @return  FADT table pointer.
> -**/
> -EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE  * -
> FindAcpiFadtTableByAcpiGuid (
> -  IN EFI_GUID  *AcpiTableGuid
> -  )
> -{
> -  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
> -  EFI_ACPI_DESCRIPTION_HEADER   *Rsdt;
> -  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
> -  UINTN Index;
> -  UINT32Data32;
> -  Rsdp  = NULL;
> -  Rsdt  = NULL;
> -  Fadt  = NULL;
> -  //
> -  // found ACPI table RSD_PTR from system table
> -  //
> -  for (Index = 0; Index < gST->NumberOfTableEntries; Index++) {
> -if (CompareGuid (&(gST->ConfigurationTable[Index].VendorGuid),
> AcpiTableGuid)) {
> -  //
> -  // A match was found.
> -  //
> -  Rsdp = gST->ConfigurationTable[Index].VendorTable;
> -  break;
> -}
> -  }
> -
> -  if (Rsdp == NULL) {
> -return NULL;
> -  }
> -
> -  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN) Rsdp->RsdtAddress;
> -  if (Rsdt == NULL || Rsdt->Signature !=
> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> -return NULL;
> -  }
> -
> -  for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < Rsdt-
> >Length; Index = Index + sizeof (UINT32)) {
> -
> -Data32  = *(UINT32 *) ((UINT8 *) Rsdt + Index);
> -Fadt= (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) (UINT32 *)
> (UINTN) Data32;
> -if (Fadt->Header.Signature ==
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> -  break;
> -}
> -  }
> -
> -  if (Fadt == NULL || Fadt->Header.Signature !=
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> -return NULL;
> -  }
> -
> -  return Fadt;
> -}
> -
> -/**
> -  To find FADT in ACPI tables.
> -
> -  @return  FADT table pointer.
> 

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

[Duran, Leo]

> -Original Message-
> From: Ni, Ruiyu 
> Sent: Wednesday, September 12, 2018 9:39 PM
> To: Duran, Leo ; Laszlo Ersek ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
> prior to MTRR change.
> 
> Leo,
> Sorry I was in leave yesterday so didn't see the mail.
> Which MSRs are shared? Only the MSR_IA32_MTRR_DEF_TYPE_REGISTER?
> Or all the MSRs that configures the CPU MTRR setting?
> 

Hi Ray,
The MTTR config MSRs are also shared by threads within a core.

> I also agree with Laszlo's comments to fix the caller if all MSRs relating to
> MTRR are shared.
> That will be to fix MpInitLib and CpuDxe driver.
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Duran, Leo 
> > Sent: Thursday, September 13, 2018 2:22 AM
> > To: Laszlo Ersek ; edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Ni, Ruiyu 
> > Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> > MTRRs prior to MTRR change.
> >
> >
> >
> > > -Original Message-
> > > From: Laszlo Ersek 
> > > Sent: Wednesday, September 12, 2018 12:59 PM
> > > To: Duran, Leo ; edk2-devel@lists.01.org
> > > Cc: Eric Dong ; Ruiyu Ni 
> > > Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> > > MTRRs prior to MTRR change.
> > >
> > > On 09/12/18 17:17, Duran, Leo wrote:
> > > > Laszlo, et al,
> > > >
> > > > Perhaps it would be best if I provide an example of the problem
> > > > I'm trying
> > > to solve, and perhaps we may chime in with suggestions.
> > > >
> > > > Again,
> > > > The fundamental issue has to do with shared MTRR control by set of
> > > threads within a core with SMT enabled.
> > > > So ideally only one thread (the first thread that wakes up) from a
> > > > set would
> > > configure the MSR, and other threads in the set would not need to.
> > > >
> > > > The problem with the existing code is that once the first thread
> > > > configures
> > > the MSR, another thread in the set follows and clears the ENABLE bit
> > > in MtrrLibPreMtrrChange().
> > >
> > > Right, I think I understood that. What I don't understand is:
> > >
> > > > (Basically, the second thread pulls the rug from under the first 
> > > > thread).
> > >
> > > why it is a problem that, when the second thread wakes up on the
> > > same core, it repeats the whole MTRR configuration? It does clear
> > > the Enable bit temporarily, yes, but in the end it re-stores the
> > > exact same MTRR config. And while this happens, the first thread on
> > > the core (already past the MTRR
> > > setup) shouldn't be doing anything at all, because all logical CPUs
> > > should sit tight until all of them complete the MTRR setup.
> > >
> > >
> > > Now, of course, if the MTRR setup runs in parallel on all logical
> > > CPUs (I can't recall off-hand whether they are started up in
> > > parallel or one-by-one -- I think it's "free for all" though), and
> > > on various CPU models this modifies resources that are shared, then
> > > we have a more serious problem. Because then two threads on the same
> > > core may modify parts of the shared MTRR settings *other* than just the
> Enable bit.
> > >
> >
> > I also think it's a 'free for all"
> > (My understanding is that the BSP issues a broadcast to the APs.)
> >
> > Perhaps if the second thread waits for the first thread to complete
> > and get safely parked, the first thread may be then immune to the
> > subsequent changes.
> >
> > > Perhaps this should be solved by adding suitable exclusion, so that
> > > only one thread on each core configure the MTRR. In other words,
> > > continue to permit full parallelism between cores, but restrict each
> > > core to just one logical thread, when doing the MTRR changes.
> > >
> >
> > Yes, agreed!
> > That would be ideal for the case where threads in a core share MTRR
> > resources.
> >
> > Unfortunately, MtrrSetAllMtrrs() gets called from quite a few places,
> > so orchestrating the "only one logical thread" logic may be quite
> complicated.
> > Again, I presume that while the worker thread makes the MTRR changes,
> > the other threads must be safely parked and immune to toggling of the
> > Enable bit.
> >
> > > I wonder if the right approach is to discover the CPU topology
> > > up-front (based on the initial APIC IDs perhaps?), and to modify the
> > > MTRR setup callback for StartupAllAPs -- on the affected CPU models
> > > -- so that the callback returns without doing anything on the
> > > initially
> > "blacklisted"
> > > threads.
> > >
> > > By the time we are in MtrrLib, it's likely too late to catch the
> > > non-favored theads.
> > >
> > > (I'd very much like others to chime in too.)
> > >
> > > Thanks
> > > Laszlo
> > >
> > > >
> > > > I hope that helps explain what I'm after.
> > > > Leo.
> > > >
> > > >> -Original Message-
> > > >> From: Laszlo Ersek 
> > > >> Sent: Wednesday, September 12, 2018 4:55 AM
> > > >> To: Duran, Leo ; edk2-devel@lists.01.org
> > > >> Cc: Eric Dong ; Ruiyu 

Re: [edk2] [PATCH 0/4] MdeModulePkg: add support for dispatching foreign arch PE/COFF images

[Kinney, Michael D]

Ard,

I think there is a fundamental assumption that
the sizeof(UINTN) and size of pointers of 
the native CPU are the same as the emulated CPU.
If that is not the case, then I would like to see
more details.  Otherwise that is a significant
restriction that needs to be clearly documented.

Protocols that only allow a single instance need to
clearly document that assumption.

If we decide to treat EBC as an emulated CPU, then
we would want to support multiple instances of the
protocol.  The updates to the DXE Core are a bit 
confusing because it has separate handling of EBC
and emulated CPUs.  I think it would make the DXE
Core logic simpler and easier to understand if they
were combined.

I asked about the startup case because if we do EBC,
then we may need more services in the protocol because
of the thunk code between native and EBC code.  At the
time EBC was originally implemented, we did not have 
paging enabled and the EBC interpreter work without 
depending on a page fault handler.

The way the protocol is currently defined, I believe it
fundamentally assumes paging is enabled.  If paging is
not enabled, then the current protocol services are not
sufficient for any emulated CPU.  Do we want this to work
for paging disabled cases?  If not, another assumption
to clearly document.

Thanks,

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, September 13, 2018 3:37 AM
> To: Kinney, Michael D 
> Cc: Ni, Ruiyu ; Zimmer, Vincent
> ; Dong, Eric
> ; edk2-devel@lists.01.org;
> ag...@suse.de; Andrew Fish ;
> Richardson, Brian ; Laszlo
> Ersek ; Zeng, Star
> 
> Subject: Re: [edk2] [PATCH 0/4] MdeModulePkg: add
> support for dispatching foreign arch PE/COFF images
> 
> On 12 September 2018 at 17:10, Kinney, Michael D
>  wrote:
> > Ard,
> >
> > I think there may be a lot of assumptions in this
> > proposal that are not documented.
> >
> > I do not see any description of how an image is
> > started or how calls between native code and emulated
> > code are handled.
> >
> 
> It is the job of the implementation of the emulator to
> use the
> information provided to it at registration time to
> ensure that calls
> to its entry point are handled. One way could be to
> remap the code
> region non-exec (which is what X86EmulatorPkg does) and
> hook the
> exceptions that result when calling into the non-native
> code. Another
> approach could be to do binary translation at load time,
> hook the
> PE/COFF entry point in the header to point to a load
> stub etc etc.
> 
> These are all implementation details of the emulator,
> and so I am not
> sure what exactly we need to document at the protocol
> level. Does the
> above paragraph capture more or less what you mean?
> 
> > Also, are multiple emulated CPUs supported?  It looks
> > like there can only be one instance of this new
> protocol.
> >
> 
> Yes. I am not sure what the added value is of having
> multiple
> emulators. Do you have anything in mind?
> 
> In any case, the singleton protocol could be implemented
> by a wrapper
> driver that encapsulates this multiple dispatch
> functionality. I don't
> think it makes sense to duplicate logic for that purpose
> in the
> various places that I touch in this series, and so it
> shouldn't affect
> the protocol prototypes.
> 
> > Can you please provide more detailed comments for the
> > functions in the new protocol and document the
> assumptions
> > and known limitation in the header?
> >
> 
> Of course. But note that they are fairly simple
> prototypes that don't
> impose any limitations by themselves, other than that
> you should call
> RegisterImage() before attempting to invoke its entry
> point.
> 
> > From one perspective, EBC is an emulated instruction
> set.
> > Would it make sense to have EBC be one of the emulated
> > CPU types that can be plugged in?
> >
> 
> We could definitely abstract away the EBC handling in
> the DXE core to
> be routed via this protocol, but that would make the
> changes more
> intrusive. I could look into making those changes on top
> if there is
> interest.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch 1/3] EmbeddedPkg/VirtualKeyboard: Avoid notification called more than once

[Ard Biesheuvel]

On 10 September 2018 at 09:12, dandan bi  wrote:
> From: Dandan Bi 
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=996
>
> Issue:
> In current code logic, when a key is pressed, it will search
> the whole NotifyList to find whether a notification has been
> registered with the keystroke. if yes, it will en-queue the
> key for notification execution later. And now if different
> notification functions have been registered with the same key,
> then the key will be en-queued more than once. Then it will
> cause the notification executed more than once.
>
> This patch is to enhance the code logic to fix this issue.
>
> Cc: Ruiyu Ni 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi 

Reviewed-by: Ard Biesheuvel 

> ---
>  EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboard.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboard.c 
> b/EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboard.c
> index 6609bc8dbe..daea9c47d2 100644
> --- a/EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboard.c
> +++ b/EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboard.c
> @@ -1,9 +1,9 @@
>  /** @file
>VirtualKeyboard driver
>
> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
>  Copyright (c) 2018, 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 which accompanies this distribution.  The
> @@ -1043,10 +1043,11 @@ VirtualKeyboardTimerHandler (
>// while current TPL is TPL_NOTIFY. It will be invoked in
>// KeyNotifyProcessHandler() which runs at TPL_CALLBACK.
>//
>Enqueue (>QueueForNotify, );
>gBS->SignalEvent (VirtualKeyboardPrivate->KeyNotifyProcessEvent);
> +  break;
>  }
>}
>
>Enqueue (>Queue, );
>
> --
> 2.14.3.windows.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 0/4] SdMmc fixes

[Wu, Hao A]

Hi Marcin,

Please give me some time to go through the series. I will provide feedbacks on 
these patches.
Sorry for the possible delay.

Best Regards,
Hao Wu

From: Marcin Wojtas [mailto:m...@semihalf.com]
Sent: Wednesday, September 12, 2018 3:29 PM
To: edk2-devel-01
Cc: Wu, Hao A; Ard Biesheuvel; Tian, Feng; Kinney, Michael D; Gao, Liming; Leif 
Lindholm; nad...@marvell.com; j...@semihalf.com; Tomasz Michalec
Subject: Re: [PATCH v2 0/4] SdMmc fixes

Hi,

So far there was one minor remark from Ard (about macro definition) - I would 
appreciate any comments before I can submit v3.

Thanks,
Marcin

pt., 7 wrz 2018 o 13:04 Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>> napisał(a):
+Hao

On 7 September 2018 at 11:10, Marcin Wojtas 
mailto:m...@semihalf.com>> wrote:
> Hi,
>
> Answering the review request, I extracted SdMmcPciHcDxe driver fixes
> from SdMmcOverride protocol modification. Comparing to v1,
> patches are rebased onto the newest master branch and also a macro
> is used instead of the raw value in SdMmcHcReset.
>
> Patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180907
>
> I'm looking forward to the comments and remarks.
>
> Best regards,
> Marcin
>
> Changelog:
> v1 -> v2
>  * rebase on top of the newest master
>  * resolve conflicts after taking fixes out from new features
>  * 3/4 - use macro instead of raw value in SdMmcHcReset
>
> Marcin Wojtas (3):
>   MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation
>   MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
>   MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot
>
> Tomasz Michalec (1):
>   MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits
>
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c| 39 +---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +--
>  4 files changed, 34 insertions(+), 39 deletions(-)
>
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation

[Wu, Hao A]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marcin Wojtas
> Sent: Friday, September 07, 2018 5:10 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; t...@semihalf.com; nad...@marvell.com; Gao, Liming; Kinney,
> Michael D
> Subject: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200
> operation
> 
> When switching to any of high speed modes (HS, HS200, HS400)
> there is need to set HS_ENABLE bit in Host Control 1 register
> which allow Host Controller to output CMD and DAT lines on
> both edges of clock. In Linux it is done after switching bus
> width in sdhci_set_ios().

I am checking both the SD and eMMC specs for this. As far as I can tell,
since the eMMC cards that support HS200 & HS400 will have 1.8V/1.2V
signaling. This makes a match within the SD specs for UHS-I mode (rather
than High Speed mode which is 3.3V signaling).

Switching the host controller to UHS-I mode requires configuring the '1.8V
Signaling Enable' (BIT3) and 'UHS Mode Select' (BIT2~0) fields of the Host
Control 2 Register. And the setting of the 'High Speed Enable' (BIT2)
field of the Host Control 1 Register seems not required to me.

Do you met with issues when switching the eMMC device to HS200/HS400 mode
when not setting BIT2 of the Host Control Register?

> 
> Also according to JESD84-B50-1 chapter 6.6.4 "HS200 timing mode
> selection" (documentation about eMMC4.5 standard) there is
> no need to disable clock when switching to HS200.

Yes, you are right that the eMMC spec does not require such a reset of the
reset SD Clock Enable (BIT2) of the Clock Control Register.

I think the code here is taking reference to the SD Host Controller
Simplified Spec 3.0:

Under the description of 'High Speed Enable' (BIT2) of the Host Control 1
Register (Table 2-16):

* If Preset Value Enable in the Host Control 2 register is set to 1, Host
* Driver needs to reset SD Clock Enable before changing this field to
* avoid generating clock glitches. After setting this field, the Host
* Driver sets SD Clock Enable again.

The spec makes very clear that the reset SD Clock Enable is only needed
when Preset Value Enable in the Host Control 2 register is set to 1.

But for the description of 'UHS Mode Select' (BIT2~0) of the Host Control
2 Register (Table 2-32):

* If Preset Value Enable in the Host Control 2 register is set to 1, Host
* Controller sets SDCLK Frequency Select, Clock Generator Select in the
* Clock Control register and Driver Strength Select according to Preset
* Value registers. In this case, one of preset value registers is selected
* by this field. Host Driver needs to reset SD Clock Enable before changing
* this field to avoid generating clock glitch. After setting this field,
* Host Driver sets SD Clock Enable again.

It's not that clear whether this reset is needed under all cases. So the
driver just play it safe here to added reset of the SD Clock Enable bit
during the switch of HS200 mode for eMMC devices.

So do you met with issues here for this? If not, I prefer to keep the
re-enabling of the clock logic here for stability consideration.

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  5 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 30 +++
> -
>  2 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index e389d52..e3fadb5 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER0xFE
> 
>  //
> +// SD Host Control 1 Register bits description
> +//
> +#define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..b3903b4 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -816,8 +816,8 @@ EmmcSwitchToHS200 (
>  {
>EFI_STATUS  Status;
>UINT8   HsTiming;
> +  UINT8   HostCtrl1;
>UINT8   HostCtrl2;
> -  UINT16  ClockCtrl;
> 
>if ((BusWidth != 4) && (BusWidth != 8)) {
>  return EFI_INVALID_PARAMETER;
> @@ -828,12 +828,10 @@ EmmcSwitchToHS200 (
>  return Status;
>}
>//
> -  // Set to HS200/SDR104 timing
> -  //
> -  //
> -  // Stop bus clock at first
> +  // Set to High Speed timing
>//
> -  Status = SdMmcHcStopClock (PciIo, Slot);
> +  HostCtrl1 = SD_MMC_HC_HOST_CTRL1_HS_ENABLE;
> +  Status = SdMmcHcOrMmio (PciIo, Slot, 

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Check HeaderType if func 0 is implemented

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 13, 2018 2:29 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen ;
> Chaganty, Rangasai V ; Chang, Tomson
> ; Huang, Jenny ; Chan,
> Amy ; Ni, Ruiyu 
> Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Check HeaderType if func 0 is
> implemented
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1169
> 
> Current code checks HeaderType of Function 0 even Function 0 is not
> implemented. HeaderType value will be 0xFF if Function 0 is not
> implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.
> 
> The code can be optimized to only check HeaderType if Function 0 is
> implemented.
> 
> Test done:
> With this patch, the result is same with the result after the patch at
> https://lists.01.org/pipermail/edk2-devel/2018-September/029623.html.
> 
> Cc: Jiewen Yao 
> Cc: Rangasai V Chaganty 
> Cc: Tomson Chang 
> Cc: Jenny Huang 
> Cc: Amy Chan 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 20
> 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> index 305995de032c..6ae5df589c1e 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> @@ -231,19 +231,13 @@ ScanPciBus (
>UINT8   HeaderType;
>UINT8   BaseClass;
>UINT8   SubClass;
> -  UINT32  MaxFunction;
>UINT16  VendorID;
>UINT16  DeviceID;
>EFI_STATUS  Status;
> 
>// Scan the PCI bus for devices
> -  for (Device = 0; Device < PCI_MAX_DEVICE + 1; Device++) {
> -HeaderType = PciSegmentRead8
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, 0,
> PCI_HEADER_TYPE_OFFSET));
> -MaxFunction = PCI_MAX_FUNC + 1;
> -if ((HeaderType & HEADER_TYPE_MULTI_FUNCTION) == 0x00) {
> -  MaxFunction = 1;
> -}
> -for (Function = 0; Function < MaxFunction; Function++) {
> +  for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
> +for (Function = 0; Function <= PCI_MAX_FUNC; Function++) {
>VendorID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function,
> PCI_VENDOR_ID_OFFSET));
>DeviceID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function,
> PCI_DEVICE_ID_OFFSET));
>if (VendorID == 0x && DeviceID == 0x) {
> @@ -275,6 +269,16 @@ ScanPciBus (
>}
>  }
>}
> +
> +  if (Function == 0) {
> +HeaderType = PciSegmentRead8
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, 0,
> PCI_HEADER_TYPE_OFFSET));
> +if ((HeaderType & HEADER_TYPE_MULTI_FUNCTION) == 0x00) {
> +  //
> +  // It is not a multi-function device, do not scan other functions.
> +  //
> +  break;
> +}
> +  }
>  }
>}
> 
> --
> 2.7.0.windows.1

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

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

[Laszlo Ersek]

On 09/13/18 14:27, Nikita Leshenko wrote:
> 
> 
>> On 11 Sep 2018, at 15:34, Laszlo Ersek  wrote:
>>
>> "BasePciExpressLib" has the prefix "Base", meaning that it is supposed
>> to be usable in all types of firmware modules, even in SEC and PEIMs --
>> which may not have access to writeable memory except stack (i.e.
>> writeable global variables). Therefore modifying the original lib
>> instance so that it depend on writeable globals wouldn't have been right.
>>
>> We could have attempted to add the new library instance under MdePkg,
>> and not ArmVirtPkg, but that's just a (more difficult) special case of
>> point (1).
>>
>> (Obviously if you try to apply the nomenclature I describe above to
>> "BaseCachingPciExpressLib" as well, you'll see that it doesn't match.
>> And that's because, when I invented the name for that lib instance, in
>> 2015, I didn't know about the naming rules myself. :) In reality the lib
>> instance should be called "DxePciExpressLibCaching" -- with "Dxe" for
>> prefix, and "Caching" for suffix.)
> 
> Thanks for the detailed explanation. I guess we have no choice
> but to copy BaseCachingPciExpressLib (renamed to
> DxePciExpressLibCaching) from ArmVirt to OVMF as well.

Uhh, hold on for a minute :) , "no choice but" is a bit of a stretch.
Where does OVMF enter the picture to begin with?

Between OvmfPkg and ArmVirtPkg, we have a sort-of rule that ArmVirtPkg
is allowed to consume OvmfPkg content, but not vice versa. So, we could
move (and rename) the lib instance from ArmVirtPkg to OvmfPkg, update
the reference(s) in ArmVirtPkg accordingly, and then consume the lib
instance in OvmfPkg as well... *if* there was any reason to consume the
lib instance in OvmfPkg in the first place.

(Of course if, by "we", you meant an internal Oracle use case, then I'm
not trying to pry. I took "we" as "upstream community". Emails are
ambiguous! :) )

> In order to prevent such bugs in the future, what do you think
> about changing the PCD reference in BasePciExpressLib.inf (in
> MdePkg) from [Pcd] to [FixedPcd]? This way a module that has a
> dynamic PcdPciExpressBaseAddress will fail to link with the
> default PCIE library. Is it practical to get such change into
> MdePkg despite their strict change policy?

I certainly think that's a valid approach if the PCD can only be Fixed
by design. However, judging by Ray's (as yet unanswered) followup
question in the thread:

734D49CCEBEEF84792F5B80ED585239D5BDF99C7@SHSMSX104.ccr.corp.intel.com">http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BDF99C7@SHSMSX104.ccr.corp.intel.com

I believe "by design" might not be a done deal just yet. Perhaps
BasePciExpressLib can be fixed to work with a dynamic PCD (and then we
should drop BaseCachingPciExpressLib in ArmVirtPkg too, and consume the
fixed BasePciExpressLib instance).

I suggest that you please file a bug for MdeModulePkg /
BasePciExpressLib in the TianoCore Bugzilla
, referencing this discussion from the
mailing list archive
,
and request that (a) either the dynamic PCD use case be fixed, or (b)
the code own up to the restriction and be explicit about FixedPCD, both
in the INF file(s) and the C-language APIs.

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

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

[Nikita Leshenko]

> On 11 Sep 2018, at 15:34, Laszlo Ersek  wrote:
> 
> "BasePciExpressLib" has the prefix "Base", meaning that it is supposed
> to be usable in all types of firmware modules, even in SEC and PEIMs --
> which may not have access to writeable memory except stack (i.e.
> writeable global variables). Therefore modifying the original lib
> instance so that it depend on writeable globals wouldn't have been right.
> 
> We could have attempted to add the new library instance under MdePkg,
> and not ArmVirtPkg, but that's just a (more difficult) special case of
> point (1).
> 
> (Obviously if you try to apply the nomenclature I describe above to
> "BaseCachingPciExpressLib" as well, you'll see that it doesn't match.
> And that's because, when I invented the name for that lib instance, in
> 2015, I didn't know about the naming rules myself. :) In reality the lib
> instance should be called "DxePciExpressLibCaching" -- with "Dxe" for
> prefix, and "Caching" for suffix.)

Thanks for the detailed explanation. I guess we have no choice
but to copy BaseCachingPciExpressLib (renamed to
DxePciExpressLibCaching) from ArmVirt to OVMF as well.

In order to prevent such bugs in the future, what do you think
about changing the PCD reference in BasePciExpressLib.inf (in
MdePkg) from [Pcd] to [FixedPcd]? This way a module that has a
dynamic PcdPciExpressBaseAddress will fail to link with the
default PCIE library. Is it practical to get such change into
MdePkg despite their strict change policy?

Nikita

> 
> Thanks
> Laszlo
> 

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

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

[Wang, Jian J]

Thanks for the explanation.

Regards,
Jian

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

On 09/12/18 02:32, Wang, Jian J wrote:
> Laszlo,
>
> Thanks for the comment. I think it’ll ok to add it in a separate patch.
>
> Just a little confuse about “a separate patch”. Does it mean a separate patch 
> file
> in the same patch series or a separate patch which needs a separate BZ 
> tracker?

Separate patch in the same series should be fine.

IMO the second patch can refer to the same BZ, or not even refer to any
BZ at all.

Thanks,
Laszlo

>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 9:52 PM
> To: Zeng, Star mailto:star.z...@intel.com>>; Wang, Jian 
> J mailto:jian.j.w...@intel.com>>; 
> edk2-devel@lists.01.org
> Cc: You, Benjamin mailto:benjamin@intel.com>>; 
> Dong, Eric mailto:eric.d...@intel.com>>
> Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config 
> error
>
> On 09/10/18 07:07, Zeng, Star wrote:
>> I agree to add the ASSERT, but even with the ASSERT, I still suggest moving
>> //
>> // Patch SmmS3ResumeState->SmmS3Cr3
>> //
>> InitSmmS3Cr3 ();
>>
>> into
>>   GuidHob = GetFirstGuidHob ();
>>   if (GuidHob != NULL) {
>> ...
>>   }
>>
>> With that, Reviewed-by: Star Zeng 
>> mailto:star.z...@intel.com>>
>
> I think that's a valid idea, but shouldn't it be done in a separate
> patch? One patch for the assert, and another moving InitSmmS3Cr3() under
> the right condition. Does that sound OK?
>
> Thanks
> Laszlo
>
>
>>
>>
>> Thanks,
>> Star
>> -Original Message-
>> From: Wang, Jian J
>> Sent: Monday, September 10, 2018 11:22 AM
>> To: 
>> edk2-devel@lists.01.org>
>> Cc: Zeng, Star 
>> mailto:star.z...@intel.com>>;
>>  You, Benjamin 
>> mailto:benjamin@intel.com>>;
>>  Dong, Eric 
>> mailto:eric.d...@intel.com>>;
>>  Laszlo Ersek 
>> mailto:ler...@redhat.com>>
>> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
>>
>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
>>
>> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if 
>> PcdAcpiS3Enable is set to TRUE. Current code in CpuS3.c doesn't embody this 
>> strong binding between them. An error message and ASSERT is added by this 
>> patch to warn platform developer about it.
>>
>> Cc: Star Zeng 
>> mailto:star.z...@intel.com>>
>> Cc: Benjamin You 
>> mailto:benjamin@intel.com>>
>> Cc: Eric Dong 
>> mailto:eric.d...@intel.com>>
>> Cc: Laszlo Ersek 
>> mailto:ler...@redhat.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang 
>> mailto:jian.j.w...@intel.com>>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> index abd8a5a07b..f371667c44 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>>  if (sizeof (UINTN) == sizeof (UINT32)) {
>>SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>>  }
>> +  } else {
>> +DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 
>> resume doesn't exist!\n"));
>> +ASSERT (FALSE);
>>}
>>
>>//
>> --
>> 2.16.2.windows.1
>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/4] MdeModulePkg/PciBusDxe: invoke PE/COFF emulator for foreign option ROMs

[Ard Biesheuvel]

On 13 September 2018 at 12:24, Zeng, Star  wrote:
> On 2018/9/12 21:21, Ard Biesheuvel wrote:
>>
>> When enumerating option ROM images, take into account whether an emulator
>> exists that would allow dispatch of PE/COFF images built for foreign
>> architectures.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>   MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h  |  1 +
>>   MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |  1 +
>>   MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 16
>> +++-
>>   3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
>> index 55eb3a5a8070..dc57d4876c0f 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
>> @@ -33,6 +33,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>> #include 
>>   #include 
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> index a21dd2b5edf4..3d99ea0c1047 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> @@ -97,6 +97,7 @@
>> gEfiLoadFile2ProtocolGuid   ## SOMETIMES_PRODUCES
>> gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES
>> gEfiLoadedImageDevicePathProtocolGuid   ## CONSUMES
>> +  gEdkiiPeCoffImageEmulatorProtocolGuid   ## SOMETIMES_CONSUMES
>> [FeaturePcd]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport  ##
>> CONSUMES
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
>> index c2be85a906af..07236afd327d 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
>> @@ -678,6 +678,7 @@ ProcessOpRomImage (
>> MEDIA_RELATIVE_OFFSET_RANGE_DEVICE_PATH  EfiOpRomImageNode;
>> VOID *Buffer;
>> UINTNBufferSize;
>> +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL *PeCoffEmulator;
>>   Indicator = 0;
>>   @@ -693,6 +694,7 @@ ProcessOpRomImage (
>> }
>> ASSERT (((EFI_PCI_EXPANSION_ROM_HEADER *) RomBarOffset)->Signature ==
>> PCI_EXPANSION_ROM_HEADER_SIGNATURE);
>>   +  PeCoffEmulator = NULL;
>> do {
>>   EfiRomHeader = (EFI_PCI_EXPANSION_ROM_HEADER *) RomBarOffset;
>>   if (EfiRomHeader->Signature != PCI_EXPANSION_ROM_HEADER_SIGNATURE) {
>> @@ -716,7 +718,19 @@ ProcessOpRomImage (
>>   // Skip the EFI PCI Option ROM image if its machine type is not
>> supported
>>   //
>>   if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED
>> (EfiRomHeader->EfiMachineType)) {
>> -  goto NextImage;
>> +  //
>> +  // Check whether we have a PE/COFF emulator that supports this
>> image
>> +  //
>> +  if (PeCoffEmulator == NULL) {
>> +gBS->LocateProtocol (,
>> NULL,
>> +   (VOID **));
>> +  }
>> +  if (PeCoffEmulator == NULL ||
>> +  !PeCoffEmulator->IsImageSupported (PeCoffEmulator,
>> + EfiRomHeader->EfiMachineType,
>> + EfiRomHeader->EfiSubsystem)) {
>> +goto NextImage;
>> +  }
>
>
> Hi Ard,
>
> Could these be abstracted to a separate function like the PATCH 4/4 did?
>

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

Re: [edk2] [PATCH V2 6/6] UefiCpuPkg PiSmmCpuDxeSmm: Use new EfiLocateFirstAcpiTable()

[Laszlo Ersek]

On 09/13/18 12:27, Star Zeng wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=967
> Request to add a library function for GetAcpiTable() in order
> to get ACPI table using signature as input.
> 
> After evaluation, we found there are many duplicated code to
> find ACPI table by signature in different modules.
> 
> This patch updates PiSmmCpuDxeSmm to use new
> EfiLocateFirstAcpiTable() and remove the duplicated code.
> 
> Cc: Younas khan 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |  4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 84 
> ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |  3 +-
>  3 files changed, 6 insertions(+), 85 deletions(-)

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 2/4] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images

[Ard Biesheuvel]

On 13 September 2018 at 12:23, Zeng, Star  wrote:
> On 2018/9/12 21:21, Ard Biesheuvel wrote:
>>
>> When encountering PE/COFF images that cannot be supported natively,
>> attempt to locate an instance of the PE/COFF image emulator protocol,
>> and if it supports the image, proceed with loading it and register it
>> with the emulator.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>   MdeModulePkg/Core/Dxe/DxeMain.h |  3 ++
>>   MdeModulePkg/Core/Dxe/DxeMain.inf   |  1 +
>>   MdeModulePkg/Core/Dxe/Image/Image.c | 39 +---
>>   3 files changed, 37 insertions(+), 6 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
>> b/MdeModulePkg/Core/Dxe/DxeMain.h
>> index 7ec82388a3f9..57b3861d9813 100644
>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>> @@ -53,6 +53,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>>   #include 
>> @@ -229,6 +230,8 @@ typedef struct {
>> UINT16  Machine;
>> /// EBC Protocol pointer
>> EFI_EBC_PROTOCOL*Ebc;
>> +  /// PE/COFF Image Emulator Protocol pointer
>> +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *Emu;
>
>
> Hi Ard,
>
> How about using PeCoffEmu as the name to be more specific?
>

Good idea.

>
>> /// Runtime image list
>> EFI_RUNTIME_IMAGE_ENTRY *RuntimeData;
>> /// Pointer to Loaded Image Device Path Protocol
>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
>> b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> index 68fa0a01d9bd..d7591aa0da6d 100644
>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> @@ -180,6 +180,7 @@
>> gEfiVariableArchProtocolGuid  ## CONSUMES
>> gEfiCapsuleArchProtocolGuid   ## CONSUMES
>> gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
>> +  gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES
>> [FeaturePcd]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ##
>> CONSUMES
>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
>> b/MdeModulePkg/Core/Dxe/Image/Image.c
>> index eddca140ee1a..e2dd80790657 100644
>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>> @@ -67,6 +67,7 @@ LOADED_IMAGE_PRIVATE_DATA mCorePrivateImage  = {
>> NULL,   // JumpContext
>> 0,  // Machine
>> NULL,   // Ebc
>> +  NULL,   // Emu
>> NULL,   // RuntimeData
>> NULL// LoadedImageDevicePath
>>   };
>> @@ -476,12 +477,23 @@ CoreLoadPeImage (
>> if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image->ImageContext.Machine)) {
>>   if (!EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED
>> (Image->ImageContext.Machine)) {
>> //
>> -  // The PE/COFF loader can support loading image types that can be
>> executed.
>> -  // If we loaded an image type that we can not execute return
>> EFI_UNSUPORTED.
>> +  // Locate the emulator protocol to check whether it supports this
>> +  // image.
>> //
>> -  DEBUG ((EFI_D_ERROR, "Image type %s can't be loaded ",
>> GetMachineTypeName(Image->ImageContext.Machine)));
>> -  DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n",
>> GetMachineTypeName(mDxeCoreImageMachineType)));
>> -  return EFI_UNSUPPORTED;
>> +  Status = CoreLocateProtocol
>> (,
>> + NULL, (VOID **)>Emu);
>> +  if (EFI_ERROR (Status) ||
>> +  !Image->Emu->IsImageSupported (Image->Emu,
>> + Image->ImageContext.Machine,
>> + Image->ImageContext.ImageType))
>> {
>> +//
>> +// The PE/COFF loader can support loading image types that can be
>> executed.
>> +// If we loaded an image type that we can not execute return
>> EFI_UNSUPORTED.
>> +//
>> +DEBUG ((EFI_D_ERROR, "Image type %s can't be loaded ",
>> GetMachineTypeName(Image->ImageContext.Machine)));
>> +DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n",
>> GetMachineTypeName(mDxeCoreImageMachineType)));
>> +return EFI_UNSUPPORTED;
>> +  }
>>   }
>> }
>>   @@ -687,6 +699,14 @@ CoreLoadPeImage (
>>   if (EFI_ERROR(Status)) {
>> goto Done;
>>   }
>> +  } else if (Image->Emu != NULL) {
>> +Status = Image->Emu->RegisterImage (Image->Emu, Image->ImageBasePage,
>> +   EFI_PAGES_TO_SIZE (Image->NumberOfPages));
>> +if (EFI_ERROR (Status)) {
>> +  DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
>> +"CoreLoadPeImage: Failed to load register foreign image with
>> emulator.\n"));
>
>
> 'load' should not be in the sentence, right?
>

Correct. I will remove it.

>> +  goto Done;
>> +}
>> }
>>   

Re: [edk2] [PATCH 1/4] MdeModulePkg: introduce PE/COFF image emulator protocol

[Ard Biesheuvel]

On 13 September 2018 at 12:05, Zeng, Star  wrote:
> On 2018/9/12 21:21, Ard Biesheuvel wrote:
>>
>> Introduce a protocol that can be invoked by the image loading services
>> to execute foreign architecture PE/COFF images via an emulator.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>   MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h | 51
>> 
>>   MdeModulePkg/MdeModulePkg.dec   |  4 ++
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
>> b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
>> new file mode 100644
>> index ..3391e68557b9
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
>> @@ -0,0 +1,51 @@
>> +/** @file
>> +  Copyright (c) 2018, 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
>> +  which accompanies this distribution.  The full text of the license may
>> be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H__
>> +#define __PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H__
>> +
>> +#define EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID \
>> +  { 0x96F46153, 0x97A7, 0x4793, { 0xAC, 0xC1, 0xFA, 0x19, 0xBF, 0x78,
>> 0xEA, 0x97 } }
>> +
>> +typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
>> EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
>> +
>> +typedef
>> +BOOLEAN
>> +(EFIAPI *IS_PECOFF_IMAGE_SUPPORTED) (
>> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
>> +  IN  UINT16  MachineType,
>> +  IN  UINT16  ImageType
>> +  );
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *REGISTER_PECOFF_IMAGE) (
>> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
>> +  IN  EFI_PHYSICAL_ADDRESSImageBase,
>> +  IN  UINT64  ImageSize
>> +  );
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *UNREGISTER_PECOFF_IMAGE) (
>> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
>> +  IN  EFI_PHYSICAL_ADDRESSImageBase
>> +  );
>> +
>> +typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL {
>> +  IS_PECOFF_IMAGE_SUPPORTED   IsImageSupported;
>> +  REGISTER_PECOFF_IMAGE   RegisterImage;
>> +  UNREGISTER_PECOFF_IMAGE UnregisterImage;
>> +} EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
>
>
> Hi Ard,
>
> There is no any comment for these protocol typedefs?
>
> How about using
> EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED/EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE/EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE
> as the function typedef names?
>
> Add below line to align with other protocol header files?
> extern EFI_GUID gEdkiiPeCoffImageEmulatorProtocolGuid;
>

Yes, I can do that.

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

Re: [edk2] [PATCH 0/4] MdeModulePkg: add support for dispatching foreign arch PE/COFF images

[Ard Biesheuvel]

On 12 September 2018 at 17:10, Kinney, Michael D
 wrote:
> Ard,
>
> I think there may be a lot of assumptions in this
> proposal that are not documented.
>
> I do not see any description of how an image is
> started or how calls between native code and emulated
> code are handled.
>

It is the job of the implementation of the emulator to use the
information provided to it at registration time to ensure that calls
to its entry point are handled. One way could be to remap the code
region non-exec (which is what X86EmulatorPkg does) and hook the
exceptions that result when calling into the non-native code. Another
approach could be to do binary translation at load time, hook the
PE/COFF entry point in the header to point to a load stub etc etc.

These are all implementation details of the emulator, and so I am not
sure what exactly we need to document at the protocol level. Does the
above paragraph capture more or less what you mean?

> Also, are multiple emulated CPUs supported?  It looks
> like there can only be one instance of this new protocol.
>

Yes. I am not sure what the added value is of having multiple
emulators. Do you have anything in mind?

In any case, the singleton protocol could be implemented by a wrapper
driver that encapsulates this multiple dispatch functionality. I don't
think it makes sense to duplicate logic for that purpose in the
various places that I touch in this series, and so it shouldn't affect
the protocol prototypes.

> Can you please provide more detailed comments for the
> functions in the new protocol and document the assumptions
> and known limitation in the header?
>

Of course. But note that they are fairly simple prototypes that don't
impose any limitations by themselves, other than that you should call
RegisterImage() before attempting to invoke its entry point.

> From one perspective, EBC is an emulated instruction set.
> Would it make sense to have EBC be one of the emulated
> CPU types that can be plugged in?
>

We could definitely abstract away the EBC handling in the DXE core to
be routed via this protocol, but that would make the changes more
intrusive. I could look into making those changes on top if there is
interest.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 is not implemented

[Zeng, Star]

Thanks.

Please also help review [PATCH] IntelSiliconPkg IntelVTdDxe: Check HeaderType 
if func 0 is implemented.


Star
-Original Message-
From: Yao, Jiewen 
Sent: Thursday, September 13, 2018 2:42 PM
To: Zeng, Star ; Ni, Ruiyu ; 
edk2-devel@lists.01.org
Cc: Chang, Tomson ; Huang, Jenny 
; Chan, Amy 
Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 
is not implemented

Sounds good.

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 13, 2018 2:30 PM
> To: Yao, Jiewen ; Ni, Ruiyu 
> ; edk2-devel@lists.01.org
> Cc: Chang, Tomson ; Huang, Jenny 
> ; Chan, Amy ; Zeng, Star 
> 
> Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when 
> func
> 0 is not implemented
> 
> Good information. :)
> The UEFI shell implementation also has the code below.
> if (PciHeader.VendorId == 0x && Func == 0) {
>   break;
> }
> 
> if (PciHeader.VendorId != 0x) {
> 
> 
> The ScanPciBus() has no functional issue, but has another optimization point.
> 
> Current code checks HeaderType of Function 0 even Function 0 is not 
> implemented. HeaderType value will be 0xFF if Function 0 is not 
> implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.
> 
> The code can be optimized to only check HeaderType if Function 0 is 
> implemented.
> 
> I just sent another patch for it at
> https://lists.01.org/pipermail/edk2-devel/2018-September/029636.html.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, September 13, 2018 1:29 PM
> To: Ni, Ruiyu ; Zeng, Star ; 
> edk2-devel@lists.01.org
> Cc: Chang, Tomson ; Huang, Jenny 
> ; Chan, Amy 
> Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when 
> func
> 0 is not implemented
> 
> I checked the UEFI shell implementation. It uses below:
> 
>   //
>   // If this is not a multi-function device, we can 
> leave the loop
>   // to deal with the next device.
>   //
>   if (Func == 0 && ((PciHeader.HeaderType &
> HEADER_TYPE_MULTI_FUNCTION) == 0x00)) {
> break;
>   }
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Thursday, September 13, 2018 11:11 AM
> > To: Zeng, Star ; edk2-devel@lists.01.org
> > Cc: Chang, Tomson ; Yao, Jiewen 
> > ; Huang, Jenny ; Chan, 
> > Amy 
> > Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize 
> > when func
> > 0 is not implemented
> >
> > On 9/13/2018 10:10 AM, Star Zeng wrote:
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1169
> > >
> > > PCI spec:
> > > They are also required to always implement function 0 in the device.
> > > Implementing other functions is optional and may be assigned in 
> > > any order (i.e., a two-function device must respond to function 0 
> > > but can choose any of the other possible function numbers (1-7) 
> > > for the second function).
> > >
> > > This patch updates ScanPciBus() to not scan other functions if 
> > > function 0 is not implemented.
> > >
> > > Test done:
> > > Added debug code below in the second loop of ScanPciBus(), 
> > > compared the debug logs with and without this patch, many
> > > non-0 unimplemented functions are skipped correctly.
> > >
> > >DEBUG ((
> > >  DEBUG_INFO,
> > >  "%a() B%02xD%02xF%02x VendorId: %04x DeviceId: %04x\n",
> > >  __FUNCTION__,
> > >  Bus,
> > >  Device,
> > >  Function,
> > >  VendorID,
> > >  DeviceID
> > >  ));
> > >
> > > Cc: Jiewen Yao 
> > > Cc: Rangasai V Chaganty 
> > > Cc: Tomson Chang 
> > > Cc: Jenny Huang 
> > > Cc: Amy Chan 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Star Zeng 
> > > ---
> > >   IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 8 +++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > index 36750b3f1d9c..305995de032c 100644
> > > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > @@ -1,6 +1,6 @@
> > >   /** @file
> > >
> > > -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights 
> > > + reserved.
> > > This program and the accompanying materials
> > > are licensed and made available under the terms and conditions 
> > > of
> > the BSD License
> > > which accompanies this distribution.  The full text of the 
> > > license may
> > be found at
> > > @@ -247,6 +247,12 @@ ScanPciBus (
> > > VendorID  = PciSegmentRead16
> > (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function, 
> > PCI_VENDOR_ID_OFFSET));
> > > DeviceID  = PciSegmentRead16
> > 

[edk2] [PATCH V2 6/6] UefiCpuPkg PiSmmCpuDxeSmm: Use new EfiLocateFirstAcpiTable()

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates PiSmmCpuDxeSmm to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |  4 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 84 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |  3 +-
 3 files changed, 6 insertions(+), 85 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index a7fb7b0b1482..0fdc1b134ea3 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -4,7 +4,7 @@
 # This SMM driver performs SMM initialization, deploy SMM Entry Vector,
 # provides CPU specific services in SMM.
 #
-# Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
+# Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
 # Copyright (c) 2017, AMD Incorporated. All rights reserved.
 #
 # This program and the accompanying materials
@@ -114,8 +114,6 @@ [Protocols]
 [Guids]
   gEfiAcpiVariableGuid ## SOMETIMES_CONSUMES ## HOB # it 
is used for S3 boot.
   gEfiGlobalVariableGuid   ## SOMETIMES_PRODUCES ## 
Variable:L"SmmProfileData"
-  gEfiAcpi20TableGuid  ## SOMETIMES_CONSUMES ## SystemTable
-  gEfiAcpi10TableGuid  ## SOMETIMES_CONSUMES ## SystemTable
   gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
   gEfiMemoryAttributesTableGuid## CONSUMES ## SystemTable
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index a743cf64f907..91b8e7ddb991 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -1,7 +1,7 @@
 /** @file
 Enable SMM profile.
 
-Copyright (c) 2012 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.
 Copyright (c) 2017, AMD Incorporated. All rights reserved.
 
 This program and the accompanying materials
@@ -726,84 +726,6 @@ InitPaging (
 }
 
 /**
-  To find FADT in ACPI tables.
-
-  @param AcpiTableGuid   The GUID used to find ACPI table in UEFI 
ConfigurationTable.
-
-  @return  FADT table pointer.
-**/
-EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE  *
-FindAcpiFadtTableByAcpiGuid (
-  IN EFI_GUID  *AcpiTableGuid
-  )
-{
-  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
-  EFI_ACPI_DESCRIPTION_HEADER   *Rsdt;
-  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
-  UINTN Index;
-  UINT32Data32;
-  Rsdp  = NULL;
-  Rsdt  = NULL;
-  Fadt  = NULL;
-  //
-  // found ACPI table RSD_PTR from system table
-  //
-  for (Index = 0; Index < gST->NumberOfTableEntries; Index++) {
-if (CompareGuid (&(gST->ConfigurationTable[Index].VendorGuid), 
AcpiTableGuid)) {
-  //
-  // A match was found.
-  //
-  Rsdp = gST->ConfigurationTable[Index].VendorTable;
-  break;
-}
-  }
-
-  if (Rsdp == NULL) {
-return NULL;
-  }
-
-  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN) Rsdp->RsdtAddress;
-  if (Rsdt == NULL || Rsdt->Signature != 
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
-return NULL;
-  }
-
-  for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < Rsdt->Length; 
Index = Index + sizeof (UINT32)) {
-
-Data32  = *(UINT32 *) ((UINT8 *) Rsdt + Index);
-Fadt= (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) (UINT32 *) (UINTN) 
Data32;
-if (Fadt->Header.Signature == 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
-  break;
-}
-  }
-
-  if (Fadt == NULL || Fadt->Header.Signature != 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
-return NULL;
-  }
-
-  return Fadt;
-}
-
-/**
-  To find FADT in ACPI tables.
-
-  @return  FADT table pointer.
-**/
-EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE  *
-FindAcpiFadtTable (
-  VOID
-  )
-{
-  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
-
-  Fadt = FindAcpiFadtTableByAcpiGuid ();
-  if (Fadt != NULL) {
-return Fadt;
-  }
-
-  return FindAcpiFadtTableByAcpiGuid ();
-}
-
-/**
   To get system port address of the SMI Command Port in FADT table.
 
 **/
@@ -814,7 +736,9 @@ GetSmiCommandPort (
 {
   EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
 
-  Fadt = FindAcpiFadtTable ();
+  Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) EfiLocateFirstAcpiTable 
(
+ 

[edk2] [PATCH V2 4/6] PcAtChipsetPkg PcRtc: Use new EfiLocateFirstAcpiTable()

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates PcatRealTimeClockRuntimeDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 80 +-
 1 file changed, 3 insertions(+), 77 deletions(-)

diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c 
b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 2105acf35f7b..7965eb8aa55b 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -1203,49 +1203,6 @@ IsWithinOneDay (
 }
 
 /**
-  This function find ACPI table with the specified signature in RSDT or XSDT.
-
-  @param Sdt  ACPI RSDT or XSDT.
-  @param SignatureACPI table signature.
-  @param TablePointerSize Size of table pointer: 4 or 8.
-
-  @return ACPI table or NULL if not found.
-**/
-VOID *
-ScanTableInSDT (
-  IN EFI_ACPI_DESCRIPTION_HEADER*Sdt,
-  IN UINT32 Signature,
-  IN UINTN  TablePointerSize
-  )
-{
-  UINTN  Index;
-  UINTN  EntryCount;
-  UINTN  EntryBase;
-  EFI_ACPI_DESCRIPTION_HEADER*Table;
-
-  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
TablePointerSize;
-
-  EntryBase = (UINTN) (Sdt + 1);
-  for (Index = 0; Index < EntryCount; Index++) {
-//
-// When TablePointerSize is 4 while sizeof (VOID *) is 8, make sure the 
upper 4 bytes are zero.
-//
-Table = 0;
-CopyMem (, (VOID *) (EntryBase + Index * TablePointerSize), 
TablePointerSize);
-
-if (Table == NULL) {
-  continue;
-}
-
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
   Get the century RTC address from the ACPI FADT table.
 
   @return  The century RTC address or 0 if not found.
@@ -1255,42 +1212,11 @@ GetCenturyRtcAddress (
   VOID
   )
 {
-  EFI_STATUSStatus;
-  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
   EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
 
-  Status = EfiGetSystemConfigurationTable (, (VOID **) 
);
-  if (EFI_ERROR (Status)) {
-Status = EfiGetSystemConfigurationTable (, (VOID **) 
);
-  }
-
-  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
-return 0;
-  }
-
-  Fadt = NULL;
-
-  //
-  // Find FADT in XSDT
-  //
-  if (Rsdp->Revision >= EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION 
&& Rsdp->XsdtAddress != 0) {
-Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINTN)
- );
-  }
-
-  //
-  // Find FADT in RSDT
-  //
-  if (Fadt == NULL && Rsdp->RsdtAddress != 0) {
-Fadt = ScanTableInSDT (
- (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress,
- EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
- sizeof (UINT32)
- );
-  }
+  Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) EfiLocateFirstAcpiTable 
(
+ 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE
+ );
 
   if ((Fadt != NULL) &&
   (Fadt->Century > RTC_ADDRESS_REGISTER_D) && (Fadt->Century < 0x80)
-- 
2.7.0.windows.1

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

[edk2] [PATCH V2 2/6] IntelSiliconPkg IntelVTdDxe: Use new EfiLocateFirstAcpiTable()

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates IntelVTdDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../Feature/VTd/IntelVTdDxe/DmarAcpiTable.c| 136 +
 1 file changed, 3 insertions(+), 133 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
index 24723fe4972a..92b09f985533 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
@@ -868,116 +868,6 @@ ParseDmarAcpiTableRmrr (
 }
 
 /**
-  This function scan ACPI table in RSDT.
-
-  @param[in]  Rsdt  ACPI RSDT
-  @param[in]  Signature ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-ScanTableInRSDT (
-  IN RSDT_TABLE   *Rsdt,
-  IN UINT32   Signature
-  )
-{
-  UINTN Index;
-  UINT32EntryCount;
-  UINT32*EntryPtr;
-  EFI_ACPI_DESCRIPTION_HEADER   *Table;
-
-  EntryCount = (Rsdt->Header.Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT32);
-
-  EntryPtr = >Entry;
-  for (Index = 0; Index < EntryCount; Index ++, EntryPtr ++) {
-Table = (EFI_ACPI_DESCRIPTION_HEADER*)((UINTN)(*EntryPtr));
-if ((Table != NULL) && (Table->Signature == Signature)) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-  This function scan ACPI table in XSDT.
-
-  @param[in]  Xsdt  ACPI XSDT
-  @param[in]  Signature ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-ScanTableInXSDT (
-  IN XSDT_TABLE   *Xsdt,
-  IN UINT32   Signature
-  )
-{
-  UINTNIndex;
-  UINT32   EntryCount;
-  UINT64   EntryPtr;
-  UINTNBasePtr;
-  EFI_ACPI_DESCRIPTION_HEADER  *Table;
-
-  EntryCount = (Xsdt->Header.Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT64);
-
-  BasePtr = (UINTN)(&(Xsdt->Entry));
-  for (Index = 0; Index < EntryCount; Index ++) {
-CopyMem (, (VOID *)(BasePtr + Index * sizeof(UINT64)), 
sizeof(UINT64));
-Table = (EFI_ACPI_DESCRIPTION_HEADER*)((UINTN)(EntryPtr));
-if ((Table != NULL) && (Table->Signature == Signature)) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-  This function scan ACPI table in RSDP.
-
-  @param[in]  Rsdp  ACPI RSDP
-  @param[in]  Signature ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-FindAcpiPtr (
-  IN EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp,
-  IN UINT32   Signature
-  )
-{
-  EFI_ACPI_DESCRIPTION_HEADER*AcpiTable;
-  RSDT_TABLE *Rsdt;
-  XSDT_TABLE *Xsdt;
-
-  AcpiTable = NULL;
-
-  //
-  // Check ACPI2.0 table
-  //
-  Rsdt = (RSDT_TABLE *)(UINTN)Rsdp->RsdtAddress;
-  Xsdt = NULL;
-  if ((Rsdp->Revision >= 2) && (Rsdp->XsdtAddress < (UINT64)(UINTN)-1)) {
-Xsdt = (XSDT_TABLE *)(UINTN)Rsdp->XsdtAddress;
-  }
-  //
-  // Check Xsdt
-  //
-  if (Xsdt != NULL) {
-AcpiTable = ScanTableInXSDT (Xsdt, Signature);
-  }
-  //
-  // Check Rsdt
-  //
-  if ((AcpiTable == NULL) && (Rsdt != NULL)) {
-AcpiTable = ScanTableInRSDT (Rsdt, Signature);
-  }
-
-  return AcpiTable;
-}
-
-/**
   Get the DMAR ACPI table.
 
   @retval EFI_SUCCESS   The DMAR ACPI table is got.
@@ -989,33 +879,13 @@ GetDmarAcpiTable (
   VOID
   )
 {
-  VOID  *AcpiTable;
-  EFI_STATUSStatus;
-
   if (mAcpiDmarTable != NULL) {
 return EFI_ALREADY_STARTED;
   }
 
-  AcpiTable = NULL;
-  Status = EfiGetSystemConfigurationTable (
- ,
- 
- );
-  if (EFI_ERROR (Status)) {
-Status = EfiGetSystemConfigurationTable (
-   ,
-   
-   );
-  }
-  if (EFI_ERROR (Status)) {
-return EFI_NOT_FOUND;
-  }
-  ASSERT (AcpiTable != NULL);
-
-  mAcpiDmarTable = FindAcpiPtr (
-  (EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER 
*)AcpiTable,
-  EFI_ACPI_4_0_DMA_REMAPPING_TABLE_SIGNATURE
-  );
+  mAcpiDmarTable = (EFI_ACPI_DMAR_HEADER *) EfiLocateFirstAcpiTable (
+  
EFI_ACPI_4_0_DMA_REMAPPING_TABLE_SIGNATURE
+  );
   if (mAcpiDmarTable == NULL) {
 return EFI_NOT_FOUND;
   }
-- 
2.7.0.windows.1

[edk2] [PATCH V2 3/6] MdeModulePkg S3SaveStateDxe: Use new EfiLocateFirstAcpiTable()

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates S3SaveStateDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Jian J Wang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../Acpi/S3SaveStateDxe/AcpiS3ContextSave.c| 208 +
 .../Acpi/S3SaveStateDxe/S3SaveStateDxe.inf |   3 +-
 2 files changed, 5 insertions(+), 206 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c 
b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
index 3f99023f110f..fadafd2b608b 100644
--- a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
+++ b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
@@ -22,8 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 
@@ -76,208 +76,6 @@ AllocateMemoryBelow4G (
 }
 
 /**
-
-  This function scan ACPI table in RSDT.
-
-  @param Rsdt  ACPI RSDT
-  @param Signature ACPI table signature
-
-  @return ACPI table
-
-**/
-VOID *
-ScanTableInRSDT (
-  IN EFI_ACPI_DESCRIPTION_HEADER*Rsdt,
-  IN UINT32 Signature
-  )
-{
-  UINTN  Index;
-  UINT32 EntryCount;
-  UINT32 *EntryPtr;
-  EFI_ACPI_DESCRIPTION_HEADER*Table;
-
-  if (Rsdt == NULL) {
-return NULL;
-  }
-
-  EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT32);
-
-  EntryPtr = (UINT32 *)(Rsdt + 1);
-  for (Index = 0; Index < EntryCount; Index ++, EntryPtr ++) {
-Table = (EFI_ACPI_DESCRIPTION_HEADER *)((UINTN)(*EntryPtr));
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-
-  This function scan ACPI table in XSDT.
-
-  @param Xsdt  ACPI XSDT
-  @param Signature ACPI table signature
-
-  @return ACPI table
-
-**/
-VOID *
-ScanTableInXSDT (
-  IN EFI_ACPI_DESCRIPTION_HEADER*Xsdt,
-  IN UINT32 Signature
-  )
-{
-  UINTN  Index;
-  UINT32 EntryCount;
-  UINT64 EntryPtr;
-  UINTN  BasePtr;
-  EFI_ACPI_DESCRIPTION_HEADER*Table;
-
-  if (Xsdt == NULL) {
-return NULL;
-  }
-
-  EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT64);
-
-  BasePtr = (UINTN)(Xsdt + 1);
-  for (Index = 0; Index < EntryCount; Index ++) {
-CopyMem (, (VOID *)(BasePtr + Index * sizeof(UINT64)), 
sizeof(UINT64));
-Table = (EFI_ACPI_DESCRIPTION_HEADER *)((UINTN)(EntryPtr));
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-  To find Facs in FADT.
-
-  @param Fadt   FADT table pointer
-
-  @return  Facs table pointer.
-**/
-EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *
-FindAcpiFacsFromFadt (
-  IN EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
-  )
-{
-  EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *Facs;
-  UINT64Data64;
-
-  if (Fadt == NULL) {
-return NULL;
-  }
-
-  if (Fadt->Header.Revision < 
EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
-Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE 
*)(UINTN)Fadt->FirmwareCtrl;
-  } else {
-if (Fadt->FirmwareCtrl != 0) {
-  Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE 
*)(UINTN)Fadt->FirmwareCtrl;
-} else {
-  CopyMem (, >XFirmwareCtrl, sizeof(UINT64));
-  Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)Data64;
-}
-  }
-  return Facs;
-}
-
-/**
-  To find Facs in Acpi tables.
-
-  To find Firmware ACPI control strutcure in Acpi Tables since the S3 waking 
vector is stored
-  in the table.
-
-  @param AcpiTableGuid   The guid used to find ACPI table in UEFI 
ConfigurationTable.
-
-  @return  Facs table pointer.
-**/
-EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *
-FindAcpiFacsTableByAcpiGuid (
-  IN EFI_GUID  *AcpiTableGuid
-  )
-{
-  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
-  EFI_ACPI_DESCRIPTION_HEADER   *Rsdt;
-  EFI_ACPI_DESCRIPTION_HEADER   *Xsdt;
-  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt;
-  EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *Facs;
-  UINTN Index;
-
-  Rsdp  = NULL;
-  //
-  // found ACPI table RSD_PTR from system table
-  //
-  for (Index = 0; Index < gST->NumberOfTableEntries; Index++) {
-if (CompareGuid (&(gST->ConfigurationTable[Index].VendorGuid), 
AcpiTableGuid)) {
- 

[edk2] [PATCH V2 5/6] ShellPkg DpDynamicCommand: Use new EfiLocateFirstAcpiTable()

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch updates DpDynamicCommand to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Dandan Bi 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c  | 136 +
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h  |   1 -
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni|   1 -
 ShellPkg/DynamicCommand/DpDynamicCommand/DpApp.inf |   2 -
 .../DpDynamicCommand/DpDynamicCommand.inf  |   2 -
 5 files changed, 3 insertions(+), 139 deletions(-)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c 
b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
index 2c094b63c94a..c14931055cbf 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
@@ -118,116 +118,6 @@ DumpStatistics( void )
 }
 
 /**
-  This function scan ACPI table in RSDT.
-
-  @param  RsdtACPI RSDT
-  @param  Signature   ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-ScanTableInRSDT (
-  IN RSDT_TABLE   *Rsdt,
-  IN UINT32   Signature
-  )
-{
-  UINTN Index;
-  UINT32EntryCount;
-  UINT32*EntryPtr;
-  EFI_ACPI_DESCRIPTION_HEADER   *Table;
-
-  EntryCount = (Rsdt->Header.Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT32);
-
-  EntryPtr = >Entry;
-  for (Index = 0; Index < EntryCount; Index ++, EntryPtr ++) {
-Table = (EFI_ACPI_DESCRIPTION_HEADER*)((UINTN)(*EntryPtr));
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-  This function scan ACPI table in XSDT.
-
-  @param  Xsdt   ACPI XSDT
-  @param  Signature  ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-ScanTableInXSDT (
-  IN XSDT_TABLE   *Xsdt,
-  IN UINT32   Signature
-  )
-{
-  UINTNIndex;
-  UINT32   EntryCount;
-  UINT64   EntryPtr;
-  UINTNBasePtr;
-  EFI_ACPI_DESCRIPTION_HEADER  *Table;
-
-  EntryCount = (Xsdt->Header.Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
sizeof(UINT64);
-
-  BasePtr = (UINTN)(&(Xsdt->Entry));
-  for (Index = 0; Index < EntryCount; Index ++) {
-CopyMem (, (VOID *)(BasePtr + Index * sizeof(UINT64)), 
sizeof(UINT64));
-Table = (EFI_ACPI_DESCRIPTION_HEADER*)((UINTN)(EntryPtr));
-if (Table->Signature == Signature) {
-  return Table;
-}
-  }
-
-  return NULL;
-}
-
-/**
-  This function scan ACPI table in RSDP.
-
-  @param  Rsdp   ACPI RSDP
-  @param  Signature  ACPI table signature
-
-  @return ACPI table
-**/
-VOID *
-FindAcpiPtr (
-  IN EFI_ACPI_5_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp,
-  IN UINT32   Signature
-  )
-{
-  EFI_ACPI_DESCRIPTION_HEADER*AcpiTable;
-  RSDT_TABLE *Rsdt;
-  XSDT_TABLE *Xsdt;
-
-  AcpiTable = NULL;
-
-  //
-  // Check ACPI2.0 table
-  //
-  Rsdt = (RSDT_TABLE *)(UINTN)Rsdp->RsdtAddress;
-  Xsdt = NULL;
-  if ((Rsdp->Revision >= 2) && (Rsdp->XsdtAddress < (UINT64)(UINTN)-1)) {
-Xsdt = (XSDT_TABLE *)(UINTN)Rsdp->XsdtAddress;
-  }
-  //
-  // Check Xsdt
-  //
-  if (Xsdt != NULL) {
-AcpiTable = ScanTableInXSDT (Xsdt, Signature);
-  }
-  //
-  // Check Rsdt
-  //
-  if ((AcpiTable == NULL) && (Rsdt != NULL)) {
-AcpiTable = ScanTableInRSDT (Rsdt, Signature);
-  }
-
-  return AcpiTable;
-}
-
-/**
   Get Boot performance table form Acpi table.
 
 **/
@@ -235,31 +125,11 @@ EFI_STATUS
 GetBootPerformanceTable (
   )
 {
-  EFI_STATUS  Status;
-  VOID*AcpiTable;
   FIRMWARE_PERFORMANCE_TABLE  *FirmwarePerformanceTable;
 
-  AcpiTable = NULL;
-
-  Status = EfiGetSystemConfigurationTable (
- ,
- 
- );
-  if (EFI_ERROR (Status)) {
-Status = EfiGetSystemConfigurationTable (
-   ,
-   
- );
-  }
-  if (EFI_ERROR(Status) || AcpiTable == NULL) {
-ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_GET_ACPI_TABLE_FAIL), 
mDpHiiHandle);
-return Status;
-  }
-
-  FirmwarePerformanceTable = FindAcpiPtr (
-  (EFI_ACPI_5_0_ROOT_SYSTEM_DESCRIPTION_POINTER 
*)AcpiTable,
-  EFI_ACPI_5_0_FIRMWARE_PERFORMANCE_DATA_TABLE_SIGNATURE
-  );
+  FirmwarePerformanceTable = (FIRMWARE_PERFORMANCE_TABLE *) 
EfiLocateFirstAcpiTable (
+   

[edk2] [PATCH V2 1/6] MdePkg UefiLib: Add new EfiLocateXXXAcpiTable() APIs

[Star Zeng]

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch adds new EfiLocateXXXAcpiTable() APIs in UefiLib
for the request and also the following patch to remove the
duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 MdePkg/Include/Library/UefiLib.h   |  68 ++
 MdePkg/Library/UefiLib/Acpi.c  | 488 +
 MdePkg/Library/UefiLib/UefiLib.inf |   3 +
 3 files changed, 559 insertions(+)
 create mode 100644 MdePkg/Library/UefiLib/Acpi.c

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index f80067f11103..cf82ff4a920a 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -26,6 +26,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #ifndef __UEFI_LIB_H__
 #define __UEFI_LIB_H__
 
+#include 
+
 #include 
 #include 
 #include 
@@ -1595,4 +1597,70 @@ EfiOpenFileByDevicePath (
   IN UINT64OpenMode,
   IN UINT64Attributes
   );
+
+/**
+  This function locates next ACPI table in XSDT/RSDT based on Signature and
+  previous returned Table.
+
+  If PreviousTable is NULL:
+  This function will locate the first ACPI table in XSDT/RSDT based on
+  Signature in gEfiAcpi20TableGuid system configuration table first, and then
+  gEfiAcpi10TableGuid system configuration table.
+  This function will locate in XSDT first, and then RSDT.
+  For DSDT, this function will locate XDsdt in FADT first, and then Dsdt in
+  FADT.
+  For FACS, this function will locate XFirmwareCtrl in FADT first, and then
+  FirmwareCtrl in FADT.
+
+  If PreviousTable is not NULL:
+  1. If it could be located in XSDT in gEfiAcpi20TableGuid system configuration
+ table, then this function will just locate next table in XSDT in
+ gEfiAcpi20TableGuid system configuration table.
+  2. If it could be located in RSDT in gEfiAcpi20TableGuid system configuration
+ table, then this function will just locate next table in RSDT in
+ gEfiAcpi20TableGuid system configuration table.
+  3. If it could be located in RSDT in gEfiAcpi10TableGuid system configuration
+ table, then this function will just locate next table in RSDT in
+ gEfiAcpi10TableGuid system configuration table.
+
+  If PreviousTable is not NULL and PreviousTable->Signature is not same with
+  Signature, then ASSERT.
+
+  @param Signature  ACPI table signature.
+  @param PreviousTable  Pointer to previous returned table to locate next
+table, or NULL to locate first table.
+
+  @return Next ACPI table or NULL if not found.
+
+**/
+EFI_ACPI_COMMON_HEADER *
+EFIAPI
+EfiLocateNextAcpiTable (
+  IN UINT32 Signature,
+  IN EFI_ACPI_COMMON_HEADER *PreviousTable OPTIONAL
+  );
+
+/**
+  This function locates first ACPI table in XSDT/RSDT based on Signature.
+
+  This function will locate the first ACPI table in XSDT/RSDT based on
+  Signature in gEfiAcpi20TableGuid system configuration table first, and then
+  gEfiAcpi10TableGuid system configuration table.
+  This function will locate in XSDT first, and then RSDT.
+  For DSDT, this function will locate XDsdt in FADT first, and then Dsdt in
+  FADT.
+  For FACS, this function will locate XFirmwareCtrl in FADT first, and then
+  FirmwareCtrl in FADT.
+
+  @param Signature  ACPI table signature.
+
+  @return First ACPI table or NULL if not found.
+
+**/
+EFI_ACPI_COMMON_HEADER *
+EFIAPI
+EfiLocateFirstAcpiTable (
+  IN UINT32 Signature
+  );
+
 #endif
diff --git a/MdePkg/Library/UefiLib/Acpi.c b/MdePkg/Library/UefiLib/Acpi.c
new file mode 100644
index ..0793bbdc787f
--- /dev/null
+++ b/MdePkg/Library/UefiLib/Acpi.c
@@ -0,0 +1,488 @@
+/** @file
+  This module provides help function for finding ACPI table.
+
+  Copyright (c) 2018, Intel Corporation. All rights reserved.
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php.
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "UefiLibInternal.h"
+#include 
+#include 
+
+/**
+  This function scans ACPI table in RSDT.
+
+  @param Rsdt   ACPI RSDT.
+  @param Signature  ACPI table signature.
+  @param PreviousTable  Pointer to previous returned table to locate
+next 

[edk2] [PATCH V2 0/6] Add new EfiLocateXXXAcpiTable() APIs

[Star Zeng]

It is the V2 patch series of
https://lists.01.org/pipermail/edk2-devel/2018-August/029214.html
It is according to the discussion at
https://lists.01.org/pipermail/edk2-devel/2018-September/029348.html

V2:
1. Add EfiLocateFirstAcpiTable() and EfiLocateNextAcpiTable() instead
   of EfiFindAcpiTableBySignature() to support locating both single
   ACPI table instance and multiple ACPI table instances cases.
2. Support locating DSDT.
3. Support locating multiple ACPI table instances case by
   EfiLocateNextAcpiTable().

Test done:
1. Call EfiLocateFirstAcpiTable() before ACPI configuration table is
   installed, NULL is returned.
2. Call EfiLocateFirstAcpiTable() to locate FACS after FACS is installed
   but FADT is not installed, NULL is returned.
3. Call EfiLocateFirstAcpiTable() to locate FADT/DSDT/FACS/FPDT/DMAR
   at late phase, correct ACPI table pointer is returned.
4. Call EfiLocateNextAcpiTable() to locate SSDTs at late phase, all
   SSDTs are returned correctly.
5. Run same test cases above after setting PcdAcpiExposedTableVersions
   to 0x2, same results are with above.

The code for this patch series is also at
g...@github.com:lzeng14/edk2.git branch LocateAcpiTable_UefiLibV2

https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.

After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.

This patch adds new EfiLocateXXXAcpiTable() API in UefiLib
for the request and removing the duplicated code.

Cc: Younas khan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jiewen Yao 
Cc: Jian J Wang 
Cc: Ruiyu Ni 
Cc: Dandan Bi 
Cc: Eric Dong 
Cc: Laszlo Ersek 

Star Zeng (6):
  MdePkg UefiLib: Add new EfiLocateXXXAcpiTable() APIs
  IntelSiliconPkg IntelVTdDxe: Use new EfiLocateFirstAcpiTable()
  MdeModulePkg S3SaveStateDxe: Use new EfiLocateFirstAcpiTable()
  PcAtChipsetPkg PcRtc: Use new EfiLocateFirstAcpiTable()
  ShellPkg DpDynamicCommand: Use new EfiLocateFirstAcpiTable()
  UefiCpuPkg PiSmmCpuDxeSmm: Use new EfiLocateFirstAcpiTable()

 .../Feature/VTd/IntelVTdDxe/DmarAcpiTable.c| 136 +-
 .../Acpi/S3SaveStateDxe/AcpiS3ContextSave.c| 208 +
 .../Acpi/S3SaveStateDxe/S3SaveStateDxe.inf |   3 +-
 MdePkg/Include/Library/UefiLib.h   |  68 +++
 MdePkg/Library/UefiLib/Acpi.c  | 488 +
 MdePkg/Library/UefiLib/UefiLib.inf |   3 +
 PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c |  80 +---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c  | 136 +-
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h  |   1 -
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni|   1 -
 ShellPkg/DynamicCommand/DpDynamicCommand/DpApp.inf |   2 -
 .../DpDynamicCommand/DpDynamicCommand.inf  |   2 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |   4 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c |  84 +---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |   3 +-
 15 files changed, 579 insertions(+), 640 deletions(-)
 create mode 100644 MdePkg/Library/UefiLib/Acpi.c

-- 
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 3/4] MdeModulePkg/PciBusDxe: invoke PE/COFF emulator for foreign option ROMs

[Zeng, Star]

On 2018/9/12 21:21, Ard Biesheuvel wrote:

When enumerating option ROM images, take into account whether an emulator
exists that would allow dispatch of PE/COFF images built for foreign
architectures.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h  |  1 +
  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |  1 +
  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 16 +++-
  3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
index 55eb3a5a8070..dc57d4876c0f 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
@@ -33,6 +33,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index a21dd2b5edf4..3d99ea0c1047 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -97,6 +97,7 @@
gEfiLoadFile2ProtocolGuid   ## SOMETIMES_PRODUCES
gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES
gEfiLoadedImageDevicePathProtocolGuid   ## CONSUMES
+  gEdkiiPeCoffImageEmulatorProtocolGuid   ## SOMETIMES_CONSUMES
  
  [FeaturePcd]

gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport  ## 
CONSUMES
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index c2be85a906af..07236afd327d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -678,6 +678,7 @@ ProcessOpRomImage (
MEDIA_RELATIVE_OFFSET_RANGE_DEVICE_PATH  EfiOpRomImageNode;
VOID *Buffer;
UINTNBufferSize;
+  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL *PeCoffEmulator;
  
Indicator = 0;
  
@@ -693,6 +694,7 @@ ProcessOpRomImage (

}
ASSERT (((EFI_PCI_EXPANSION_ROM_HEADER *) RomBarOffset)->Signature == 
PCI_EXPANSION_ROM_HEADER_SIGNATURE);
  
+  PeCoffEmulator = NULL;

do {
  EfiRomHeader = (EFI_PCI_EXPANSION_ROM_HEADER *) RomBarOffset;
  if (EfiRomHeader->Signature != PCI_EXPANSION_ROM_HEADER_SIGNATURE) {
@@ -716,7 +718,19 @@ ProcessOpRomImage (
  // Skip the EFI PCI Option ROM image if its machine type is not supported
  //
  if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (EfiRomHeader->EfiMachineType)) {
-  goto NextImage;
+  //
+  // Check whether we have a PE/COFF emulator that supports this image
+  //
+  if (PeCoffEmulator == NULL) {
+gBS->LocateProtocol (, NULL,
+   (VOID **));
+  }
+  if (PeCoffEmulator == NULL ||
+  !PeCoffEmulator->IsImageSupported (PeCoffEmulator,
+ EfiRomHeader->EfiMachineType,
+ EfiRomHeader->EfiSubsystem)) {
+goto NextImage;
+  }


Hi Ard,

Could these be abstracted to a separate function like the PATCH 4/4 did?

Thanks,
Star


  }
  
  //




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

Re: [edk2] [PATCH 2/4] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images

[Zeng, Star]

On 2018/9/12 21:21, Ard Biesheuvel wrote:

When encountering PE/COFF images that cannot be supported natively,
attempt to locate an instance of the PE/COFF image emulator protocol,
and if it supports the image, proceed with loading it and register it
with the emulator.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Core/Dxe/DxeMain.h |  3 ++
  MdeModulePkg/Core/Dxe/DxeMain.inf   |  1 +
  MdeModulePkg/Core/Dxe/Image/Image.c | 39 +---
  3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 7ec82388a3f9..57b3861d9813 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -53,6 +53,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -229,6 +230,8 @@ typedef struct {
UINT16  Machine;
/// EBC Protocol pointer
EFI_EBC_PROTOCOL*Ebc;
+  /// PE/COFF Image Emulator Protocol pointer
+  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *Emu;


Hi Ard,

How about using PeCoffEmu as the name to be more specific?


/// Runtime image list
EFI_RUNTIME_IMAGE_ENTRY *RuntimeData;
/// Pointer to Loaded Image Device Path Protocol
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 68fa0a01d9bd..d7591aa0da6d 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -180,6 +180,7 @@
gEfiVariableArchProtocolGuid  ## CONSUMES
gEfiCapsuleArchProtocolGuid   ## CONSUMES
gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
+  gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES
  
  [FeaturePcd]

gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
b/MdeModulePkg/Core/Dxe/Image/Image.c
index eddca140ee1a..e2dd80790657 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -67,6 +67,7 @@ LOADED_IMAGE_PRIVATE_DATA mCorePrivateImage  = {
NULL,   // JumpContext
0,  // Machine
NULL,   // Ebc
+  NULL,   // Emu
NULL,   // RuntimeData
NULL// LoadedImageDevicePath
  };
@@ -476,12 +477,23 @@ CoreLoadPeImage (
if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image->ImageContext.Machine)) {
  if (!EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED 
(Image->ImageContext.Machine)) {
//
-  // The PE/COFF loader can support loading image types that can be 
executed.
-  // If we loaded an image type that we can not execute return 
EFI_UNSUPORTED.
+  // Locate the emulator protocol to check whether it supports this
+  // image.
//
-  DEBUG ((EFI_D_ERROR, "Image type %s can't be loaded ", 
GetMachineTypeName(Image->ImageContext.Machine)));
-  DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n", 
GetMachineTypeName(mDxeCoreImageMachineType)));
-  return EFI_UNSUPPORTED;
+  Status = CoreLocateProtocol (,
+ NULL, (VOID **)>Emu);
+  if (EFI_ERROR (Status) ||
+  !Image->Emu->IsImageSupported (Image->Emu,
+ Image->ImageContext.Machine,
+ Image->ImageContext.ImageType)) {
+//
+// The PE/COFF loader can support loading image types that can be 
executed.
+// If we loaded an image type that we can not execute return 
EFI_UNSUPORTED.
+//
+DEBUG ((EFI_D_ERROR, "Image type %s can't be loaded ", 
GetMachineTypeName(Image->ImageContext.Machine)));
+DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n", 
GetMachineTypeName(mDxeCoreImageMachineType)));
+return EFI_UNSUPPORTED;
+  }
  }
}
  
@@ -687,6 +699,14 @@ CoreLoadPeImage (

  if (EFI_ERROR(Status)) {
goto Done;
  }
+  } else if (Image->Emu != NULL) {
+Status = Image->Emu->RegisterImage (Image->Emu, Image->ImageBasePage,
+   EFI_PAGES_TO_SIZE (Image->NumberOfPages));
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
+"CoreLoadPeImage: Failed to load register foreign image with 
emulator.\n"));


'load' should not be in the sentence, right?

Thanks,
Star


+  goto Done;
+}
}
  
//

@@ -874,6 +894,13 @@ CoreUnloadAndCloseImage (
  Image->Ebc->UnloadImage (Image->Ebc, Image->Handle);
}
  
+  if (Image->Emu != NULL) {

+//
+// If the PE/COFF Emulator protocol exists we must unregister the image.
+//
+Image->Emu->UnregisterImage (Image->Emu, Image->ImageBasePage);
+  }
+
//
// Unload image, free Image->ImageContext->ModHandle
//
@@ -1599,7 +1626,7 @@ 

Re: [edk2] [PATCH 1/4] MdeModulePkg: introduce PE/COFF image emulator protocol

[Zeng, Star]

On 2018/9/12 21:21, Ard Biesheuvel wrote:

Introduce a protocol that can be invoked by the image loading services
to execute foreign architecture PE/COFF images via an emulator.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h | 51 
  MdeModulePkg/MdeModulePkg.dec   |  4 ++
  2 files changed, 55 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h 
b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
new file mode 100644
index ..3391e68557b9
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
@@ -0,0 +1,51 @@
+/** @file
+  Copyright (c) 2018, 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
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H__
+#define __PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H__
+
+#define EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID \
+  { 0x96F46153, 0x97A7, 0x4793, { 0xAC, 0xC1, 0xFA, 0x19, 0xBF, 0x78, 0xEA, 
0x97 } }
+
+typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL 
EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
+
+typedef
+BOOLEAN
+(EFIAPI *IS_PECOFF_IMAGE_SUPPORTED) (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
+  IN  UINT16  MachineType,
+  IN  UINT16  ImageType
+  );
+
+typedef
+EFI_STATUS
+(EFIAPI *REGISTER_PECOFF_IMAGE) (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
+  IN  EFI_PHYSICAL_ADDRESSImageBase,
+  IN  UINT64  ImageSize
+  );
+
+typedef
+EFI_STATUS
+(EFIAPI *UNREGISTER_PECOFF_IMAGE) (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL*This,
+  IN  EFI_PHYSICAL_ADDRESSImageBase
+  );
+
+typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL {
+  IS_PECOFF_IMAGE_SUPPORTED   IsImageSupported;
+  REGISTER_PECOFF_IMAGE   RegisterImage;
+  UNREGISTER_PECOFF_IMAGE UnregisterImage;
+} EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;


Hi Ard,

There is no any comment for these protocol typedefs?

How about using 
EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED/EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE/EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE 
as the function typedef names?


Add below line to align with other protocol header files?
extern EFI_GUID gEdkiiPeCoffImageEmulatorProtocolGuid;

Thanks,
Star


+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 6a6d9660edc2..be307329f901 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -617,6 +617,10 @@
  
## Include/Protocol/AtaAtapiPolicy.h

gEdkiiAtaAtapiPolicyProtocolGuid = { 0xe59cd769, 0x5083, 0x4f26,{ 0x90, 
0x94, 0x6c, 0x91, 0x9f, 0x91, 0x6c, 0x4e } }
+
+  ## Include/Protocol/PeCoffImageEmulator.h
+  gEdkiiPeCoffImageEmulatorProtocolGuid = { 0x96f46153, 0x97a7, 0x4793, { 
0xac, 0xc1, 0xfa, 0x19, 0xbf, 0x78, 0xea, 0x97 } }
+
  #
  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
  #   0x8001 | Invalid value provided.



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

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

[Ni, Ruiyu]

Please ignore this one. It's an incomplete patch. Please check V3.

Thanks/Ray

> -Original Message-
> From: edk2-devel  On Behalf Of Ruiyu Ni
> Sent: Thursday, September 13, 2018 4:30 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Yao, Jiewen
> ; Gao, Liming 
> Subject: [edk2] [PATCH v2] MdePkg/SynchronizationLib: fix
> Interlocked[De|In]crement return value
> 
> Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
> perform atomic increment/decrement but doesn't guarantee the return
> value equals to the new value.
> 
> The patch fixes the behavior to use "XADD" instruction to guarantee the
> return value equals to the new value.
> 
> The patch calls intrinsic functions for MSVC tool chain, calls the NASM
> implementation for INTEL tool chain and calls GCC inline assembly
> implementation (GccInline.c) for GCC tool chain.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Jiewen Yao 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> ---
>  MdePkg/Include/Library/SynchronizationLib.h|  6 +--
>  .../BaseSynchronizationLib.inf | 18 -
>  .../BaseSynchronizationLibInternals.h  |  6 +--
>  .../BaseSynchronizationLib/Ia32/GccInline.c| 18 -
>  .../Ia32/InterlockedDecrement.c| 42 -
>  .../Ia32/InterlockedDecrement.nasm | 10 ++---
>  .../Ia32/InterlockedIncrement.c| 43 
> --
>  .../Ia32/InterlockedIncrement.nasm |  7 ++--
>  .../BaseSynchronizationLib/Synchronization.c   |  6 +--
>  .../BaseSynchronizationLib/SynchronizationGcc.c|  6 +--
>  .../BaseSynchronizationLib/SynchronizationMsc.c|  6 +--
>  .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +-
>  .../X64/InterlockedDecrement.nasm  |  8 ++--
>  .../X64/InterlockedIncrement.nasm  |  5 ++-
>  14 files changed, 52 insertions(+), 148 deletions(-)  delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
>  delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
> 
> diff --git a/MdePkg/Include/Library/SynchronizationLib.h
> b/MdePkg/Include/Library/SynchronizationLib.h
> index da69f6ff5e..ce3bce04f5 100644
> --- a/MdePkg/Include/Library/SynchronizationLib.h
> +++ b/MdePkg/Include/Library/SynchronizationLib.h
> @@ -144,8 +144,7 @@ ReleaseSpinLock (
> 
>Performs an atomic increment of the 32-bit unsigned integer specified by
>Value and returns the incremented value. The increment operation must
> be
> -  performed using MP safe mechanisms. The state of the return value is not
> -  guaranteed to be MP safe.
> +  performed using MP safe mechanisms.
> 
>If Value is NULL, then ASSERT().
> 
> @@ -166,8 +165,7 @@ InterlockedIncrement (
> 
>Performs an atomic decrement of the 32-bit unsigned integer specified by
>Value and returns the decremented value. The decrement operation must
> be
> -  performed using MP safe mechanisms. The state of the return value is not
> -  guaranteed to be MP safe.
> +  performed using MP safe mechanisms.
> 
>If Value is NULL, then ASSERT().
> 
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> index 0be1d4331f..427e959f2e 100755
> --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> @@ -34,9 +34,9 @@ [Sources.IA32]
>Ia32/InterlockedCompareExchange64.c | MSFT
>Ia32/InterlockedCompareExchange32.c | MSFT
>Ia32/InterlockedCompareExchange16.c | MSFT
> -  Ia32/InterlockedDecrement.c | MSFT
> -  Ia32/InterlockedIncrement.c | MSFT
> -  SynchronizationMsc.c  | MSFT
> +  InterlockedIncrementMsc.c | MSFT
> +  InterlockedDecrementMsc.c | MSFT
> +  SynchronizationMsc.c | MSFT
> 
>Ia32/InterlockedCompareExchange64.nasm| INTEL
>Ia32/InterlockedCompareExchange32.nasm| INTEL @@ -54,17 +54,15 @@
> [Sources.X64]
>X64/InterlockedCompareExchange64.c | MSFT
>X64/InterlockedCompareExchange32.c | MSFT
>X64/InterlockedCompareExchange16.c | MSFT
> +  InterlockedIncrementMsc.c | MSFT
> +  InterlockedDecrementMsc.c | MSFT
> +  SynchronizationMsc.c | MSFT
> 
>X64/InterlockedCompareExchange64.nasm| INTEL
>X64/InterlockedCompareExchange32.nasm| INTEL
>X64/InterlockedCompareExchange16.nasm| INTEL
> -
> -  X64/InterlockedDecrement.c | MSFT
> -  X64/InterlockedIncrement.c | MSFT
> -  SynchronizationMsc.c | MSFT
> -
> -  X64/InterlockedDecrement.nasm| INTEL
> -  X64/InterlockedIncrement.nasm| INTEL
> +  X64/InterlockedDecrement.nasm | INTEL  X64/InterlockedIncrement.nasm
> + | INTEL
>Synchronization.c | INTEL
> 
>Ia32/InternalGetSpinLockProperties.c | GCC diff --git
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.
> h
> 

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

[Ruiyu Ni]

Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
perform atomic increment/decrement but doesn't guarantee the return
value equals to the new value.

The patch fixes the behavior to use "XADD" instruction to guarantee
the return value equals to the new value.

The patch calls intrinsic functions for MSVC tool chain, calls the
NASM implementation for INTEL tool chain and calls GCC inline
assembly implementation (GccInline.c) for GCC tool chain.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Jiewen Yao 
Cc: Liming Gao 
Cc: Michael D Kinney 
---
 MdePkg/Include/Library/SynchronizationLib.h|  6 +--
 .../BaseSynchronizationLib.inf | 18 -
 .../BaseSynchronizationLibInternals.h  |  6 +--
 .../BaseSynchronizationLib/Ia32/GccInline.c| 18 -
 .../Ia32/InterlockedDecrement.c| 42 -
 .../Ia32/InterlockedDecrement.nasm | 10 ++---
 .../Ia32/InterlockedIncrement.c| 43 --
 .../Ia32/InterlockedIncrement.nasm |  7 ++--
 ...lockedDecrement.c => InterlockedDecrementMsc.c} |  4 +-
 ...lockedIncrement.c => InterlockedIncrementMsc.c} |  4 +-
 .../BaseSynchronizationLib/Synchronization.c   |  6 +--
 .../BaseSynchronizationLib/SynchronizationGcc.c|  6 +--
 .../BaseSynchronizationLib/SynchronizationMsc.c|  6 +--
 .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +-
 .../X64/InterlockedDecrement.nasm  |  8 ++--
 .../X64/InterlockedIncrement.nasm  |  5 ++-
 16 files changed, 56 insertions(+), 152 deletions(-)
 delete mode 100644 
MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
 delete mode 100644 
MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
 rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedDecrement.c => 
InterlockedDecrementMsc.c} (87%)
 rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedIncrement.c => 
InterlockedIncrementMsc.c} (87%)

diff --git a/MdePkg/Include/Library/SynchronizationLib.h 
b/MdePkg/Include/Library/SynchronizationLib.h
index da69f6ff5e..ce3bce04f5 100644
--- a/MdePkg/Include/Library/SynchronizationLib.h
+++ b/MdePkg/Include/Library/SynchronizationLib.h
@@ -144,8 +144,7 @@ ReleaseSpinLock (
 
   Performs an atomic increment of the 32-bit unsigned integer specified by
   Value and returns the incremented value. The increment operation must be
-  performed using MP safe mechanisms. The state of the return value is not
-  guaranteed to be MP safe.
+  performed using MP safe mechanisms.
 
   If Value is NULL, then ASSERT().
 
@@ -166,8 +165,7 @@ InterlockedIncrement (
 
   Performs an atomic decrement of the 32-bit unsigned integer specified by
   Value and returns the decremented value. The decrement operation must be
-  performed using MP safe mechanisms. The state of the return value is not
-  guaranteed to be MP safe.
+  performed using MP safe mechanisms.
 
   If Value is NULL, then ASSERT().
 
diff --git a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf 
b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
index 0be1d4331f..427e959f2e 100755
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
@@ -34,9 +34,9 @@ [Sources.IA32]
   Ia32/InterlockedCompareExchange64.c | MSFT
   Ia32/InterlockedCompareExchange32.c | MSFT
   Ia32/InterlockedCompareExchange16.c | MSFT
-  Ia32/InterlockedDecrement.c | MSFT
-  Ia32/InterlockedIncrement.c | MSFT
-  SynchronizationMsc.c  | MSFT
+  InterlockedIncrementMsc.c | MSFT
+  InterlockedDecrementMsc.c | MSFT
+  SynchronizationMsc.c | MSFT
 
   Ia32/InterlockedCompareExchange64.nasm| INTEL
   Ia32/InterlockedCompareExchange32.nasm| INTEL
@@ -54,17 +54,15 @@ [Sources.X64]
   X64/InterlockedCompareExchange64.c | MSFT
   X64/InterlockedCompareExchange32.c | MSFT
   X64/InterlockedCompareExchange16.c | MSFT
+  InterlockedIncrementMsc.c | MSFT
+  InterlockedDecrementMsc.c | MSFT
+  SynchronizationMsc.c | MSFT
 
   X64/InterlockedCompareExchange64.nasm| INTEL
   X64/InterlockedCompareExchange32.nasm| INTEL
   X64/InterlockedCompareExchange16.nasm| INTEL
-
-  X64/InterlockedDecrement.c | MSFT
-  X64/InterlockedIncrement.c | MSFT
-  SynchronizationMsc.c | MSFT
-
-  X64/InterlockedDecrement.nasm| INTEL
-  X64/InterlockedIncrement.nasm| INTEL
+  X64/InterlockedDecrement.nasm | INTEL
+  X64/InterlockedIncrement.nasm | INTEL
   Synchronization.c | INTEL
 
   Ia32/InternalGetSpinLockProperties.c | GCC
diff --git 
a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h 
b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
index 37edd7c188..8c363a8585 100644
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
+++ 

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

[Ruiyu Ni]

Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
perform atomic increment/decrement but doesn't guarantee the return
value equals to the new value.

The patch fixes the behavior to use "XADD" instruction to guarantee
the return value equals to the new value.

The patch calls intrinsic functions for MSVC tool chain, calls the
NASM implementation for INTEL tool chain and calls GCC inline
assembly implementation (GccInline.c) for GCC tool chain.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Jiewen Yao 
Cc: Liming Gao 
Cc: Michael D Kinney 
---
 MdePkg/Include/Library/SynchronizationLib.h|  6 +--
 .../BaseSynchronizationLib.inf | 18 -
 .../BaseSynchronizationLibInternals.h  |  6 +--
 .../BaseSynchronizationLib/Ia32/GccInline.c| 18 -
 .../Ia32/InterlockedDecrement.c| 42 -
 .../Ia32/InterlockedDecrement.nasm | 10 ++---
 .../Ia32/InterlockedIncrement.c| 43 --
 .../Ia32/InterlockedIncrement.nasm |  7 ++--
 .../BaseSynchronizationLib/Synchronization.c   |  6 +--
 .../BaseSynchronizationLib/SynchronizationGcc.c|  6 +--
 .../BaseSynchronizationLib/SynchronizationMsc.c|  6 +--
 .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +-
 .../X64/InterlockedDecrement.nasm  |  8 ++--
 .../X64/InterlockedIncrement.nasm  |  5 ++-
 14 files changed, 52 insertions(+), 148 deletions(-)
 delete mode 100644 
MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
 delete mode 100644 
MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c

diff --git a/MdePkg/Include/Library/SynchronizationLib.h 
b/MdePkg/Include/Library/SynchronizationLib.h
index da69f6ff5e..ce3bce04f5 100644
--- a/MdePkg/Include/Library/SynchronizationLib.h
+++ b/MdePkg/Include/Library/SynchronizationLib.h
@@ -144,8 +144,7 @@ ReleaseSpinLock (
 
   Performs an atomic increment of the 32-bit unsigned integer specified by
   Value and returns the incremented value. The increment operation must be
-  performed using MP safe mechanisms. The state of the return value is not
-  guaranteed to be MP safe.
+  performed using MP safe mechanisms.
 
   If Value is NULL, then ASSERT().
 
@@ -166,8 +165,7 @@ InterlockedIncrement (
 
   Performs an atomic decrement of the 32-bit unsigned integer specified by
   Value and returns the decremented value. The decrement operation must be
-  performed using MP safe mechanisms. The state of the return value is not
-  guaranteed to be MP safe.
+  performed using MP safe mechanisms.
 
   If Value is NULL, then ASSERT().
 
diff --git a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf 
b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
index 0be1d4331f..427e959f2e 100755
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
@@ -34,9 +34,9 @@ [Sources.IA32]
   Ia32/InterlockedCompareExchange64.c | MSFT
   Ia32/InterlockedCompareExchange32.c | MSFT
   Ia32/InterlockedCompareExchange16.c | MSFT
-  Ia32/InterlockedDecrement.c | MSFT
-  Ia32/InterlockedIncrement.c | MSFT
-  SynchronizationMsc.c  | MSFT
+  InterlockedIncrementMsc.c | MSFT
+  InterlockedDecrementMsc.c | MSFT
+  SynchronizationMsc.c | MSFT
 
   Ia32/InterlockedCompareExchange64.nasm| INTEL
   Ia32/InterlockedCompareExchange32.nasm| INTEL
@@ -54,17 +54,15 @@ [Sources.X64]
   X64/InterlockedCompareExchange64.c | MSFT
   X64/InterlockedCompareExchange32.c | MSFT
   X64/InterlockedCompareExchange16.c | MSFT
+  InterlockedIncrementMsc.c | MSFT
+  InterlockedDecrementMsc.c | MSFT
+  SynchronizationMsc.c | MSFT
 
   X64/InterlockedCompareExchange64.nasm| INTEL
   X64/InterlockedCompareExchange32.nasm| INTEL
   X64/InterlockedCompareExchange16.nasm| INTEL
-
-  X64/InterlockedDecrement.c | MSFT
-  X64/InterlockedIncrement.c | MSFT
-  SynchronizationMsc.c | MSFT
-
-  X64/InterlockedDecrement.nasm| INTEL
-  X64/InterlockedIncrement.nasm| INTEL
+  X64/InterlockedDecrement.nasm | INTEL
+  X64/InterlockedIncrement.nasm | INTEL
   Synchronization.c | INTEL
 
   Ia32/InternalGetSpinLockProperties.c | GCC
diff --git 
a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h 
b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
index 37edd7c188..8c363a8585 100644
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
@@ -27,8 +27,7 @@
 
   Performs an atomic increment of the 32-bit unsigned integer specified by
   Value and returns the incremented value. The increment operation must be
-  performed using MP safe mechanisms. The state of the return value is not
-  guaranteed to be MP safe.
+  performed using MP safe mechanisms.
 
   @param 

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 is not implemented

[Yao, Jiewen]

Sounds good.

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 13, 2018 2:30 PM
> To: Yao, Jiewen ; Ni, Ruiyu ;
> edk2-devel@lists.01.org
> Cc: Chang, Tomson ; Huang, Jenny
> ; Chan, Amy ; Zeng, Star
> 
> Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func
> 0 is not implemented
> 
> Good information. :)
> The UEFI shell implementation also has the code below.
> if (PciHeader.VendorId == 0x && Func == 0) {
>   break;
> }
> 
> if (PciHeader.VendorId != 0x) {
> 
> 
> The ScanPciBus() has no functional issue, but has another optimization point.
> 
> Current code checks HeaderType of Function 0 even Function 0 is not
> implemented. HeaderType value will be 0xFF if Function 0 is not
> implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.
> 
> The code can be optimized to only check HeaderType if Function 0 is
> implemented.
> 
> I just sent another patch for it at
> https://lists.01.org/pipermail/edk2-devel/2018-September/029636.html.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, September 13, 2018 1:29 PM
> To: Ni, Ruiyu ; Zeng, Star ;
> edk2-devel@lists.01.org
> Cc: Chang, Tomson ; Huang, Jenny
> ; Chan, Amy 
> Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func
> 0 is not implemented
> 
> I checked the UEFI shell implementation. It uses below:
> 
>   //
>   // If this is not a multi-function device, we can leave
> the loop
>   // to deal with the next device.
>   //
>   if (Func == 0 && ((PciHeader.HeaderType &
> HEADER_TYPE_MULTI_FUNCTION) == 0x00)) {
> break;
>   }
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Thursday, September 13, 2018 11:11 AM
> > To: Zeng, Star ; edk2-devel@lists.01.org
> > Cc: Chang, Tomson ; Yao, Jiewen
> > ; Huang, Jenny ; Chan,
> > Amy 
> > Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when
> > func
> > 0 is not implemented
> >
> > On 9/13/2018 10:10 AM, Star Zeng wrote:
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1169
> > >
> > > PCI spec:
> > > They are also required to always implement function 0 in the device.
> > > Implementing other functions is optional and may be assigned in any
> > > order (i.e., a two-function device must respond to function 0 but
> > > can choose any of the other possible function numbers (1-7) for the
> > > second function).
> > >
> > > This patch updates ScanPciBus() to not scan other functions if
> > > function 0 is not implemented.
> > >
> > > Test done:
> > > Added debug code below in the second loop of ScanPciBus(), compared
> > > the debug logs with and without this patch, many
> > > non-0 unimplemented functions are skipped correctly.
> > >
> > >DEBUG ((
> > >  DEBUG_INFO,
> > >  "%a() B%02xD%02xF%02x VendorId: %04x DeviceId: %04x\n",
> > >  __FUNCTION__,
> > >  Bus,
> > >  Device,
> > >  Function,
> > >  VendorID,
> > >  DeviceID
> > >  ));
> > >
> > > Cc: Jiewen Yao 
> > > Cc: Rangasai V Chaganty 
> > > Cc: Tomson Chang 
> > > Cc: Jenny Huang 
> > > Cc: Amy Chan 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Star Zeng 
> > > ---
> > >   IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 8 +++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > index 36750b3f1d9c..305995de032c 100644
> > > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > > @@ -1,6 +1,6 @@
> > >   /** @file
> > >
> > > -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights
> > > + reserved.
> > > This program and the accompanying materials
> > > are licensed and made available under the terms and conditions
> > > of
> > the BSD License
> > > which accompanies this distribution.  The full text of the
> > > license may
> > be found at
> > > @@ -247,6 +247,12 @@ ScanPciBus (
> > > VendorID  = PciSegmentRead16
> > (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function,
> > PCI_VENDOR_ID_OFFSET));
> > > DeviceID  = PciSegmentRead16
> > (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function,
> > PCI_DEVICE_ID_OFFSET));
> > > if (VendorID == 0x && DeviceID == 0x) {
> > > +if (Function == 0) {
> > > +  //
> > > +  // If function 0 is not implemented, do not scan other
> > functions.
> > > +  //
> > > +  break;
> > > +}
> > >   continue;
> > > }
> > >
> > >
> > Reviewed-by: Ruiyu Ni 
> >
> > 

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 is not implemented

[Zeng, Star]

Good information. :)
The UEFI shell implementation also has the code below.
if (PciHeader.VendorId == 0x && Func == 0) {
  break;
}

if (PciHeader.VendorId != 0x) {


The ScanPciBus() has no functional issue, but has another optimization point.

Current code checks HeaderType of Function 0 even Function 0 is not
implemented. HeaderType value will be 0xFF if Function 0 is not
implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.

The code can be optimized to only check HeaderType if Function 0 is
implemented.

I just sent another patch for it at 
https://lists.01.org/pipermail/edk2-devel/2018-September/029636.html.


Thanks,
Star
-Original Message-
From: Yao, Jiewen 
Sent: Thursday, September 13, 2018 1:29 PM
To: Ni, Ruiyu ; Zeng, Star ; 
edk2-devel@lists.01.org
Cc: Chang, Tomson ; Huang, Jenny 
; Chan, Amy 
Subject: RE: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when func 0 
is not implemented

I checked the UEFI shell implementation. It uses below:

  //
  // If this is not a multi-function device, we can leave the 
loop
  // to deal with the next device.
  //
  if (Func == 0 && ((PciHeader.HeaderType & 
HEADER_TYPE_MULTI_FUNCTION) == 0x00)) {
break;
  }

Thank you
Yao Jiewen

> -Original Message-
> From: Ni, Ruiyu
> Sent: Thursday, September 13, 2018 11:11 AM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Chang, Tomson ; Yao, Jiewen 
> ; Huang, Jenny ; Chan, 
> Amy 
> Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Optimize when 
> func
> 0 is not implemented
> 
> On 9/13/2018 10:10 AM, Star Zeng wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1169
> >
> > PCI spec:
> > They are also required to always implement function 0 in the device.
> > Implementing other functions is optional and may be assigned in any 
> > order (i.e., a two-function device must respond to function 0 but 
> > can choose any of the other possible function numbers (1-7) for the 
> > second function).
> >
> > This patch updates ScanPciBus() to not scan other functions if 
> > function 0 is not implemented.
> >
> > Test done:
> > Added debug code below in the second loop of ScanPciBus(), compared 
> > the debug logs with and without this patch, many
> > non-0 unimplemented functions are skipped correctly.
> >
> >DEBUG ((
> >  DEBUG_INFO,
> >  "%a() B%02xD%02xF%02x VendorId: %04x DeviceId: %04x\n",
> >  __FUNCTION__,
> >  Bus,
> >  Device,
> >  Function,
> >  VendorID,
> >  DeviceID
> >  ));
> >
> > Cc: Jiewen Yao 
> > Cc: Rangasai V Chaganty 
> > Cc: Tomson Chang 
> > Cc: Jenny Huang 
> > Cc: Amy Chan 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng 
> > ---
> >   IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 8 +++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > index 36750b3f1d9c..305995de032c 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
> > @@ -1,6 +1,6 @@
> >   /** @file
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights 
> > + reserved.
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions 
> > of
> the BSD License
> > which accompanies this distribution.  The full text of the 
> > license may
> be found at
> > @@ -247,6 +247,12 @@ ScanPciBus (
> > VendorID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function, 
> PCI_VENDOR_ID_OFFSET));
> > DeviceID  = PciSegmentRead16
> (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, Device, Function, 
> PCI_DEVICE_ID_OFFSET));
> > if (VendorID == 0x && DeviceID == 0x) {
> > +if (Function == 0) {
> > +  //
> > +  // If function 0 is not implemented, do not scan other
> functions.
> > +  //
> > +  break;
> > +}
> >   continue;
> > }
> >
> >
> Reviewed-by: Ruiyu Ni 
> 
> --
> Thanks,
> Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Check HeaderType if func 0 is implemented

[Star Zeng]

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

Current code checks HeaderType of Function 0 even Function 0 is not
implemented. HeaderType value will be 0xFF if Function 0 is not
implemented, then MaxFunction will be set to PCI_MAX_FUNC + 1.

The code can be optimized to only check HeaderType if Function 0 is
implemented.

Test done:
With this patch, the result is same with the result after the patch at
https://lists.01.org/pipermail/edk2-devel/2018-September/029623.html.

Cc: Jiewen Yao 
Cc: Rangasai V Chaganty 
Cc: Tomson Chang 
Cc: Jenny Huang 
Cc: Amy Chan 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
index 305995de032c..6ae5df589c1e 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/PciInfo.c
@@ -231,19 +231,13 @@ ScanPciBus (
   UINT8   HeaderType;
   UINT8   BaseClass;
   UINT8   SubClass;
-  UINT32  MaxFunction;
   UINT16  VendorID;
   UINT16  DeviceID;
   EFI_STATUS  Status;
 
   // Scan the PCI bus for devices
-  for (Device = 0; Device < PCI_MAX_DEVICE + 1; Device++) {
-HeaderType = PciSegmentRead8 (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, 
Device, 0, PCI_HEADER_TYPE_OFFSET));
-MaxFunction = PCI_MAX_FUNC + 1;
-if ((HeaderType & HEADER_TYPE_MULTI_FUNCTION) == 0x00) {
-  MaxFunction = 1;
-}
-for (Function = 0; Function < MaxFunction; Function++) {
+  for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
+for (Function = 0; Function <= PCI_MAX_FUNC; Function++) {
   VendorID  = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, 
Device, Function, PCI_VENDOR_ID_OFFSET));
   DeviceID  = PciSegmentRead16 (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, 
Device, Function, PCI_DEVICE_ID_OFFSET));
   if (VendorID == 0x && DeviceID == 0x) {
@@ -275,6 +269,16 @@ ScanPciBus (
   }
 }
   }
+
+  if (Function == 0) {
+HeaderType = PciSegmentRead8 (PCI_SEGMENT_LIB_ADDRESS(Segment, Bus, 
Device, 0, PCI_HEADER_TYPE_OFFSET));
+if ((HeaderType & HEADER_TYPE_MULTI_FUNCTION) == 0x00) {
+  //
+  // It is not a multi-function device, do not scan other functions.
+  //
+  break;
+}
+  }
 }
   }
 
-- 
2.7.0.windows.1

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