[edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[Isaac Oram]

The S3FeaturePkg changes in
  [edk2-platforms][PATCH v3 2/4] S3FeaturePkg: Implement working S3 resume

Introduces some build issues with standalone package build for S3FeaturePkg and
AdvancedFeaturePkg.  There are also some type cast related compiler warnings.

We do not currently have continuous integration testing.
We do not currently have documented build testing configuration requirements.
Therefore I am just fixing the minor issues and intend to merge both patch
series together to maintain git bisect to the best of my ability.
I do plan to document required and recommended board port and feature pkg
builds.

Note that the use of UINTN for intermediate data instead of
EFI_PHYSICAL_ADDRESS is only to be consistent with other ACPI implementations
of similar functionality.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>

Isaac Oram (2):
  S3FeaturePkg/Build: Add libraries needed by S3FeaturePkg
  MinPlatformPkg/S3: Use EFI_PHYSICAL_ADDRESS for address

 .../Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc    |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c   | 10 +++++-----
 .../PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc      |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c   |  2 +-
 .../Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h  |  4 ++--
 5 files changed, 14 insertions(+), 8 deletions(-)

--
2.36.1.windows.1

[edk2-devel][edk2-platforms][PATCH V1 1/2] S3FeaturePkg/Build: Add libraries needed by S3FeaturePkg

[Isaac Oram]

This change is needed for proposed S3FeaturePkg additions.
Changes ensure standalone build for S3FeaturePkg and AdvancedFeaturePkg

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
---
 Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc     | 3 +++
 Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc b/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
index ce0eac7a56..18929589c5 100644
--- a/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
+++ b/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
@@ -115,6 +115,9 @@
 [LibraryClasses.Common]
   PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf # Required by BeepDebugFeature
 
+[LibraryClasses.Common.PEIM]
+  SmmAccessLib|IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.inf # Required by S3Feature
+
 #
 # This package builds all advanced features.
 #
diff --git a/Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc b/Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc
index cd4b1c4f19..d5efcf6b6d 100644
--- a/Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc
+++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc
@@ -41,6 +41,9 @@
 !include MinPlatformPkg/Include/Dsc/CorePeiLib.dsc
 !include MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
 
+[LibraryClasses]
+  PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
+
 [LibraryClasses.common.PEIM]
   SmmAccessLib|IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.inf
 
-- 
2.36.1.windows.1

[edk2-devel][edk2-platforms][PATCH V1 2/2] MinPlatformPkg/S3: Use EFI_PHYSICAL_ADDRESS for address

[Isaac Oram]

This change fixes build warnings with type casts from some compilers.
Also changes type for ACPI_S3_MEMORY S3PeiMemBase to EFI_PHYSICAL_ADDRESS.
This generally makes code eaiser to understand as opposed to UINT64.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
---
 .../Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c   | 10 +++++-----
 .../Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c   |  2 +-
 .../Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h  |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c
index 1a7ccb8eed..47b2cc27a5 100644
--- a/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c
+++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c
@@ -124,19 +124,19 @@ S3DxeEntryPoint (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  UINT64          S3PeiMemSize;
-  UINT64          S3PeiMemBase;
+  UINTN           S3PeiMemSize;
+  UINTN           S3PeiMemBase;
   ACPI_S3_MEMORY  S3MemoryInfo;
   EFI_STATUS      Status;
 
   DEBUG ((DEBUG_INFO, "%a() Start\n", __FUNCTION__));
 
-  S3PeiMemSize = GetPeiMemSize ();
+  S3PeiMemSize = (UINTN) GetPeiMemSize ();
   S3PeiMemBase = (UINTN) AllocateAcpiNvsMemoryBelow4G (S3PeiMemSize);
   ASSERT (S3PeiMemBase != 0);
 
-  S3MemoryInfo.S3PeiMemBase = S3PeiMemBase;
-  S3MemoryInfo.S3PeiMemSize = S3PeiMemSize;
+  S3MemoryInfo.S3PeiMemBase = (EFI_PHYSICAL_ADDRESS) S3PeiMemBase;
+  S3MemoryInfo.S3PeiMemSize = (UINT64) S3PeiMemSize;
 
   DEBUG ((DEBUG_INFO, "S3PeiMemBase: 0x%x\n", S3PeiMemBase));
   DEBUG ((DEBUG_INFO, "S3PeiMemSize: 0x%x\n", S3PeiMemSize));
diff --git a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c
index 6acb894b6f..388e0954bd 100644
--- a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c
+++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c
@@ -40,7 +40,7 @@ FspSiliconInitDoneNotify (
 {
   EFI_STATUS     Status;
   EFI_BOOT_MODE  BootMode;
-  UINT64         MchBaseAddress;
+  UINTN          MchBaseAddress;
 
   Status = PeiServicesGetBootMode (&BootMode);
   ASSERT_EFI_ERROR (Status);
diff --git a/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h b/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h
index 04eb1435ee..35943e2aa3 100644
--- a/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h
+++ b/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h
@@ -13,8 +13,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // NV data structure
 //
 typedef struct {
-  UINT64  S3PeiMemBase;
-  UINT64  S3PeiMemSize;
+  EFI_PHYSICAL_ADDRESS  S3PeiMemBase;
+  UINT64                S3PeiMemSize;
 } ACPI_S3_MEMORY;
 
 #define ACPI_S3_MEMORY_NV_NAME  L"S3MemoryInfo"
-- 
2.36.1.windows.1

Re: [edk2-devel] [edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[Benjamin Doron]

[-- Attachment #1: Type: text/plain, Size: 94 bytes --]

Thanks, Isaac. Are these two S3 series ready to merge, or did I have more review to address?

[-- Attachment #2: Type: text/html, Size: 94 bytes --]

Re: [edk2-devel] [edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[Isaac Oram]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

Nate or Sai, this series should be a quick review if you could please prioritize…

Benjamin,

Your series is reviewed but breaks several builds due to unrelated changes.  My plan is to get approval for mine, then submit both series as a batch to ensure bisect not broken.
But I didn’t get a reviewed by or ack yet for my changes.  If you review/send Reviewed-by then all I need is an Ack.  And it may be that the others were waiting for your approval since my changes modify yours.
Anytime I am included in a review, but am not a maintainer, I try to review as it isn’t approval to commit, but maintainers may want to see my thoughts.  That is my interpretation anyway.  There are no requirement for you to review, maintainer approval is the gate as I understand it.

There are two paths.  But if you integrate my changes into yours and send a new version, it is then signed off by both of us and we need a third reviewer anyway.  Thus it seems easier to have Nate or Sai review my series.

Regards.
Isaac

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, September 28, 2022 9:17 AM
To: Oram, Isaac W <isaac.w.oram@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

Thanks, Isaac. Are these two S3 series ready to merge, or did I have more review to address?


[-- Attachment #2: Type: text/html, Size: 3940 bytes --]

Re: [edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[Nate DeSimone]

For the series...

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Oram, Isaac W <isaac.w.oram@intel.com> 
Sent: Wednesday, September 14, 2022 11:40 AM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Chiu, Chasel <chasel.chiu@intel.com>; Dong, Eric <eric.dong@intel.com>; Benjamin Doron <benjamin.doron00@gmail.com>
Subject: [edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

The S3FeaturePkg changes in
  [edk2-platforms][PATCH v3 2/4] S3FeaturePkg: Implement working S3 resume

Introduces some build issues with standalone package build for S3FeaturePkg and AdvancedFeaturePkg.  There are also some type cast related compiler warnings.

We do not currently have continuous integration testing.
We do not currently have documented build testing configuration requirements.
Therefore I am just fixing the minor issues and intend to merge both patch series together to maintain git bisect to the best of my ability.
I do plan to document required and recommended board port and feature pkg builds.

Note that the use of UINTN for intermediate data instead of EFI_PHYSICAL_ADDRESS is only to be consistent with other ACPI implementations of similar functionality.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>

Isaac Oram (2):
  S3FeaturePkg/Build: Add libraries needed by S3FeaturePkg
  MinPlatformPkg/S3: Use EFI_PHYSICAL_ADDRESS for address

 .../Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc    |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c   | 10 +++++-----
 .../PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc      |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c   |  2 +-
 .../Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h  |  4 ++--
 5 files changed, 14 insertions(+), 8 deletions(-)

--
2.36.1.windows.1

Re: [edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

[Isaac Oram]

Pushed as 07d0c989089f94133e7b71e38c18d206134863e7

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
Sent: Tuesday, October 11, 2022 4:40 PM
To: Oram, Isaac W <isaac.w.oram@intel.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Chiu, Chasel <chasel.chiu@intel.com>; Dong, Eric <eric.dong@intel.com>; Benjamin Doron <benjamin.doron00@gmail.com>
Subject: RE: [edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

For the series...

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Oram, Isaac W <isaac.w.oram@intel.com> 
Sent: Wednesday, September 14, 2022 11:40 AM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Chiu, Chasel <chasel.chiu@intel.com>; Dong, Eric <eric.dong@intel.com>; Benjamin Doron <benjamin.doron00@gmail.com>
Subject: [edk2-devel][edk2-platforms][PATCH V1 0/2] Platforms/Intel: Build fixes

The S3FeaturePkg changes in
  [edk2-platforms][PATCH v3 2/4] S3FeaturePkg: Implement working S3 resume

Introduces some build issues with standalone package build for S3FeaturePkg and AdvancedFeaturePkg.  There are also some type cast related compiler warnings.

We do not currently have continuous integration testing.
We do not currently have documented build testing configuration requirements.
Therefore I am just fixing the minor issues and intend to merge both patch series together to maintain git bisect to the best of my ability.
I do plan to document required and recommended board port and feature pkg builds.

Note that the use of UINTN for intermediate data instead of EFI_PHYSICAL_ADDRESS is only to be consistent with other ACPI implementations of similar functionality.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>

Isaac Oram (2):
  S3FeaturePkg/Build: Add libraries needed by S3FeaturePkg
  MinPlatformPkg/S3: Use EFI_PHYSICAL_ADDRESS for address

 .../Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc    |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c   | 10 +++++-----
 .../PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc      |  3 +++
 .../Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c   |  2 +-
 .../Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h  |  4 ++--
 5 files changed, 14 insertions(+), 8 deletions(-)

--
2.36.1.windows.1