[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

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-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 --]

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

[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

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] -=-=-=-=-=-=

[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 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-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-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 --]

[edk2-devel][edk2-platforms][PATCH v1 0/4] Platform/Intel:

[Benjamin Doron]

BoardAcpiDxe is derived from AcpiPlatformDxe, but only the
silicon/platform DSDT is installed. However, boards can have tables that
must be installed too. For instance, laptops often have an EC ACPI
device. Therefore, implement support for installing the generic
PcdAcpiTableStorageFile too.

Tested with KabylakeOpenBoardPkg, the Aspire VN7-572G's BoardAcpiTables
are successfully installed.

Benjamin Doron (4):
  KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile
  CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile
  MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile
  WhiskeylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile

 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 .../Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 10 ++++++++++
 .../Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf |  3 +++
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 8 files changed, 61 insertions(+), 3 deletions(-)

-- 
2.37.2

[edk2-devel][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>
---
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..ba59c84fd4ba 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -270,7 +270,20 @@ InstallAcpiBoard (
   //
   // Platform ACPI Tables
   //
-  PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  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
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
 
   //
   // 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.37.2