[edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 0361ffc2fbf9..fd227d9b410e 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -289,6 +289,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables
   //
   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
 
   //
   // This protocol publish must be done after PublishAcpiTablesFromFv.
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 7d2e105e5447..4d7f5cdb3eeb 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
@@ -59,6 +59,8 @@
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.31.1

[edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables
   //
   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
 
   //
   // This protocol publish must be done after PublishAcpiTablesFromFv.
diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
@@ -62,6 +62,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.31.1

[edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 1 +
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
index 9bc222453857..417cccc18b5a 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
@@ -227,6 +227,7 @@ InstallMinDsdt (
 
   Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
   ASSERT_EFI_ERROR (Status);
+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
   
   return EFI_SUCCESS;
 }
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
index 3437bc489cd3..3e14c4e9a65d 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
@@ -41,6 +41,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid
-- 
2.31.1

[edk2-platforms][PATCH v1 4/4] WhiskeylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index cb5f328a3909..f7bad8025aa7 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables
   //
   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
 
   //
   // This protocol publish must be done after PublishAcpiTablesFromFv.
diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index af5c8f1c0609..f17fb588ddb8 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
@@ -60,6 +60,8 @@
   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints
   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.31.1

Re: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Chaganty, Rangasai V]

Hi Benjamin, 
The problem statement is not clear to me.
Could you share any background for this change - RFC, Bugzilla etc?

Thanks,
Sai Chaganty

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Wednesday, August 04, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Kethi Reddy, Deepika <deepika.kethi.reddy@intel.com>; Esakkithevar, Kathappan <kathappan.esakkithevar@intel.com>
Subject: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables   //   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));    //   // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.inf
@@ -62,6 +62,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid     AND-- 
2.31.1

Re: [edk2-devel] [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

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

Hi Sai,
There may be a requirement to install board-specific ACPI tables. For example, to support an embedded controller in laptops, or other controllers in servers and desktops.

In a conversation with Nate about a board port in KabylakeOpenBoardPkg, I said that the options for such boards would be to include AcpiPlatformDxe, or, since BoardAcpiDxe seems to be a superset of AcpiPlatformDxe functionality, add it here by calling PublishAcpiTablesFromFv() again with this GUID.

Perhaps an RFC first would have been best. I don't mind dropping this patchset, but can we wait for comments first?

Best regards,
Benjamin

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

Re: [edk2-devel] [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

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

Another thing that was interesting to me is that PcdAcpiTableStorageFile is actually defined in MdeModulePkg and used in MdeModulePkg/Universal/Acpi/AcpiPlatformDxe. Our MinPlatform board packages do not use that driver, we have our own BoardAcpiDxe that varies with each silicon generation. For that reason I’d be willing to call it a core feature that we have not implemented correctly in our MinPlatform *OpenBoardPkg’s.

It may be a good idea to see if MdeModulePkg/Universal/Acpi/AcpiPlatformDxe is usable in the MinPlatform *OpenBoardPkg’s but that seems like a task for another time.

Thanks,
Nate

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, August 4, 2021 2:51 PM
To: Chaganty; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Hi Sai,
There may be a requirement to install board-specific ACPI tables. For example, to support an embedded controller in laptops, or other controllers in servers and desktops.

In a conversation with Nate about a board port in KabylakeOpenBoardPkg, I said that the options for such boards would be to include AcpiPlatformDxe, or, since BoardAcpiDxe seems to be a superset of AcpiPlatformDxe functionality, add it here by calling PublishAcpiTablesFromFv() again with this GUID.

Perhaps an RFC first would have been best. I don't mind dropping this patchset, but can we wait for comments first?

Best regards,
Benjamin


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

Re: [edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

Hi Benjamin,

It would be good to add a check on the status code returned from PublishAcpiTablesFromFv() to the debug log. Something like this:

  Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Error: PublishAcpiTablesFromFv[%g] - %r", &gEfiCallerIdGuid, Status));
  }
  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
  if (EFI_ERROR (Status)) {
    DEBUG ((
      DEBUG_ERROR,
      "Error: PublishAcpiTablesFromFv[%g] - %r",
      PcdGetPtr (PcdAcpiTableStorageFile),
      Status
      ));
  }

I know the original code doesn't do it, but I imagine this will be helpful to some poor soul who is trying to debug issues with this later on.

Thanks,
Nate

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 0361ffc2fbf9..fd227d9b410e 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe
+++ .c
@@ -289,6 +289,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables   //   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));    //   // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 7d2e105e5447..4d7f5cdb3eeb 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe
+++ .inf
@@ -59,6 +59,8 @@
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid     AND-- 
2.31.1

Re: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

Hi Benjamin,

It would be good to add a check on the status code returned from PublishAcpiTablesFromFv() to the debug log. Something like this:

  Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Error: PublishAcpiTablesFromFv[%g] - %r", &gEfiCallerIdGuid, Status));
  }
  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
  if (EFI_ERROR (Status)) {
    DEBUG ((
      DEBUG_ERROR,
      "Error: PublishAcpiTablesFromFv[%g] - %r",
      PcdGetPtr (PcdAcpiTableStorageFile),
      Status
      ));
  }

I know the original code doesn't do it, but I imagine this will be helpful to some poor soul who is trying to debug issues with this later on.

Thanks,
Nate

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Kethi Reddy, Deepika <deepika.kethi.reddy@intel.com>; Esakkithevar, Kathappan <kathappan.esakkithevar@intel.com>
Subject: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables   //   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));    //   // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.inf
@@ -62,6 +62,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid     AND-- 
2.31.1

Re: [edk2-platforms][PATCH v1 4/4] WhiskeylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

Hi Benjamin,

It would be good to add a check on the status code returned from PublishAcpiTablesFromFv() to the debug log. Something like this:

  Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Error: PublishAcpiTablesFromFv[%g] - %r", &gEfiCallerIdGuid, Status));
  }
  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
  if (EFI_ERROR (Status)) {
    DEBUG ((
      DEBUG_ERROR,
      "Error: PublishAcpiTablesFromFv[%g] - %r",
      PcdGetPtr (PcdAcpiTableStorageFile),
      Status
      ));
  }

I know the original code doesn't do it, but I imagine this will be helpful to some poor soul who is trying to debug issues with this later on.

Thanks,
Nate

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms][PATCH v1 4/4] WhiskeylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c   | 1 +
 Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index cb5f328a3909..f7bad8025aa7 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpi
+++ Dxe.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
   // Platform ACPI Tables   //   PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));    //   // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index af5c8f1c0609..f17fb588ddb8 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpi
+++ Dxe.inf
@@ -60,6 +60,8 @@
   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid     AND-- 
2.31.1

Re: [edk2-devel] [edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

Hi Benjamin,

It would be good to add a check on the status code returned from PublishAcpiTablesFromFv() to the debug log. Something like this:

  Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Error: PublishAcpiTablesFromFv[%g] - %r", &gEfiCallerIdGuid, Status));
  }
  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
  if (EFI_ERROR (Status)) {
    DEBUG ((
      DEBUG_ERROR,
      "Error: PublishAcpiTablesFromFv[%g] - %r",
      PcdGetPtr (PcdAcpiTableStorageFile),
      Status
      ));
  }

I know the original code doesn't do it, but I imagine this will be helpful to some poor soul who is trying to debug issues with this later on.

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 1 +
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
index 9bc222453857..417cccc18b5a 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
@@ -227,6 +227,7 @@ InstallMinDsdt (
    Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);   ASSERT_EFI_ERROR (Status);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));      return EFI_SUCCESS; }diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
index 3437bc489cd3..3e14c4e9a65d 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
@@ -41,6 +41,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid-- 
2.31.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78665): https://edk2.groups.io/g/devel/message/78665
Mute This Topic: https://groups.io/mt/84670596/1767664
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [nathaniel.l.desimone@intel.com] -=-=-=-=-=-=

Re: [edk2-devel] [edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

[Nate DeSimone]

Hi Benjamin,

This is a compilation error in this patch. You have added the usage of gEfiMdeModulePkgTokenSpaceGuid to MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf but that GUID technically does not exist when this .inf is being compiled since the .inf only references the following DEC files:

  MdePkg/MdePkg.dec
  MinPlatformPkg/MinPlatformPkg.dec

You need to add MdeModulePkg/MdeModulePkg.dec

I suspect you missed this error because all the currently existing OpenBoardPkgs do not use MinDsdt thus far.

Thanks,
Nate 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 1 +
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
index 9bc222453857..417cccc18b5a 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
@@ -227,6 +227,7 @@ InstallMinDsdt (
    Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);   ASSERT_EFI_ERROR (Status);+  PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));      return EFI_SUCCESS; }diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
index 3437bc489cd3..3e14c4e9a65d 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
@@ -41,6 +41,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex]   gEfiAcpiTableProtocolGuid           AND   gEfiFirmwareVolume2ProtocolGuid-- 
2.31.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78665): https://edk2.groups.io/g/devel/message/78665
Mute This Topic: https://groups.io/mt/84670596/1767664
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [nathaniel.l.desimone@intel.com] -=-=-=-=-=-=

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

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

Hi Nate,
Thanks for the feedback on this patchset, I've addressed it in V2.

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

Re: [edk2-devel] [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

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

Another reason to install these tables with BoardAcpiDxe is that it calls a board-implemented hook to patch the tables, if necessary. AcpiPlatformDxe only installs the tables as they are found.

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

[edk2-devel][edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile

[Benjamin Doron]

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 10 ++++++++++
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
index 9bc222453857..ac343afca7d4 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
@@ -227,6 +227,16 @@ InstallMinDsdt (
 
   Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
   ASSERT_EFI_ERROR (Status);
+  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "Error: PublishAcpiTablesFromFv[%g] - %r",
+      PcdGetPtr (PcdAcpiTableStorageFile),
+      Status
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
   
   return EFI_SUCCESS;
 }
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
index 3437bc489cd3..dc32186eb361 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
@@ -21,6 +21,7 @@
 
 [Packages]
   MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   MinPlatformPkg/MinPlatformPkg.dec
 
 [LibraryClasses]
@@ -41,6 +42,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid
-- 
2.37.2