* [PATCH v2 0/6] Enhance DynamicTablesPkg modules
@ 2022-07-28 4:31 Kun Qin
2022-07-28 4:31 ` [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf Kun Qin
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Pierre Gondois
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
This patch series is a follow-up of previous submission:
https://edk2.groups.io/g/devel/message/91497
The main changes between v1 and v2 patches are:
- Added reviewed-by collected from previous iteration
- Updated mandatory tables presence verification routine
- Added checks of config space before creating RES0 nodes
Current DynamicTablesPkg provide great support for creating dynamic ACPI
tables during boot time.
However, there are some modules needs minor tweaks to expand support and
compatibility for OS requirements and platform needs.
This patch series proposes a few fixes to resolve minor issues discovered
in DynamicPlatRepoLib, AcpiSsdtPcieLibArm and DynamicTableManagerDxe.
Patch v2 branch: https://github.com/kuqin12/edk2/tree/dynamic_update_v2
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>
Kun Qin (6):
DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf
DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing
DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers
DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed
tables
DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM
space
DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI
config
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 174 +++++++++++++++++++
DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c | 80 ++++++++-
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf | 1 +
5 files changed, 354 insertions(+), 84 deletions(-)
--
2.37.1.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
2022-07-28 4:31 ` [PATCH v2 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing Kun Qin
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez, Sami Mujawar,
Pierre Gondois
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996
The DynamicPlatRepoLib has multiple reference to MemoryAllocationLib,
such as DynamicPlatRepo.c and TokenMapper.c. Not including it in the
library inf file could lead to potential build break.
This change added the MemoryAllocationLib into this inf file.
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---
Notes:
v2:
- Added Reviewed-by tag [Sami]
- Added Reviewed-by tag [Pierre]
DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf | 1 +
1 file changed, 1 insertion(+)
diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
index 9a3cc87fd91d..8423352550c2 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
@@ -31,3 +31,4 @@ [Packages]
[LibraryClasses]
AcpiHelperLib
BaseLib
+ MemoryAllocationLib
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
2022-07-28 4:31 ` [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
2022-07-28 4:31 ` [PATCH v2 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers Kun Qin
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez, Sami Mujawar,
Pierre Gondois
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996
The content of token should be derived from the data section of the
`CmObject` instead of the object itself.
This change fixed the issue by dereferencing the token value from the
data buffer of input CmObject.
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---
Notes:
v2:
- Added Reviewed-by tag [Sami]
- Added Reviewed-by tag [Pierre]
DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
index 80d0aa17bc1a..84e4bb7e3bc8 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
@@ -60,7 +60,7 @@ TokenFixerItsGroup (
)
{
ASSERT (CmObject != NULL);
- ((CM_ARM_ITS_GROUP_NODE *)CmObject)->Token = Token;
+ ((CM_ARM_ITS_GROUP_NODE *)CmObject->Data)->Token = Token;
return EFI_SUCCESS;
}
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
2022-07-28 4:31 ` [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf Kun Qin
2022-07-28 4:31 ` [PATCH v2 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
2022-07-28 4:31 ` [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables Kun Qin
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez, Sami Mujawar,
Pierre Gondois
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996
This change added more token fixers for other node types, including
NamedComponentNode, RootComplexNode, and SmmuV3Node.
The corresponding entries for tokenFixer functions table is also updated.
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---
Notes:
v2:
- Added Reviewed-by tag [Sami]
- Added Reviewed-by tag [Pierre]
DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c | 78 +++++++++++++++++++-
1 file changed, 75 insertions(+), 3 deletions(-)
diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
index 84e4bb7e3bc8..345acab53f74 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
@@ -64,6 +64,78 @@ TokenFixerItsGroup (
return EFI_SUCCESS;
}
+/** EArmObjNamedComponent token fixer.
+
+ CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+ @param [in] CmObject Pointer to the Configuration Manager Object.
+ @param [in] Token Token to be updated in the CmObject.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_UNSUPPORTED Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerNamedComponentNode (
+ IN CM_OBJ_DESCRIPTOR *CmObject,
+ IN CM_OBJECT_TOKEN Token
+ )
+{
+ ASSERT (CmObject != NULL);
+ ((CM_ARM_NAMED_COMPONENT_NODE *)CmObject->Data)->Token = Token;
+ return EFI_SUCCESS;
+}
+
+/** EArmObjRootComplex token fixer.
+
+ CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+ @param [in] CmObject Pointer to the Configuration Manager Object.
+ @param [in] Token Token to be updated in the CmObject.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_UNSUPPORTED Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerRootComplexNode (
+ IN CM_OBJ_DESCRIPTOR *CmObject,
+ IN CM_OBJECT_TOKEN Token
+ )
+{
+ ASSERT (CmObject != NULL);
+ ((CM_ARM_ROOT_COMPLEX_NODE *)CmObject->Data)->Token = Token;
+ return EFI_SUCCESS;
+}
+
+/** EArmObjSmmuV3 token fixer.
+
+ CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+ @param [in] CmObject Pointer to the Configuration Manager Object.
+ @param [in] Token Token to be updated in the CmObject.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_UNSUPPORTED Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerSmmuV3Node (
+ IN CM_OBJ_DESCRIPTOR *CmObject,
+ IN CM_OBJECT_TOKEN Token
+ )
+{
+ ASSERT (CmObject != NULL);
+ ((CM_ARM_SMMUV3_NODE *)CmObject->Data)->Token = Token;
+ return EFI_SUCCESS;
+}
+
/** TokenFixer functions table.
A CmObj having a CM_OBJECT_TOKEN field might need to have its
@@ -90,10 +162,10 @@ CM_OBJECT_TOKEN_FIXER TokenFixer[EArmObjMax] = {
NULL, ///< 16 - Hypervisor Vendor Id
NULL, ///< 17 - Fixed feature flags for FADT
TokenFixerItsGroup, ///< 18 - ITS Group
- TokenFixerNotImplemented, ///< 19 - Named Component
- TokenFixerNotImplemented, ///< 20 - Root Complex
+ TokenFixerNamedComponentNode, ///< 19 - Named Component
+ TokenFixerRootComplexNode, ///< 20 - Root Complex
TokenFixerNotImplemented, ///< 21 - SMMUv1 or SMMUv2
- TokenFixerNotImplemented, ///< 22 - SMMUv3
+ TokenFixerSmmuV3Node, ///< 22 - SMMUv3
TokenFixerNotImplemented, ///< 23 - PMCG
NULL, ///< 24 - GIC ITS Identifier Array
NULL, ///< 25 - ID Mapping Array
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
` (2 preceding siblings ...)
2022-07-28 4:31 ` [PATCH v2 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
2022-07-28 4:31 ` [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space Kun Qin
2022-07-28 4:31 ` [PATCH v2 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config Kun Qin
5 siblings, 1 reply; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
This change added an extra step to allow check for installed ACPI tables.
For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either pre-installed or
supplied through AcpiTableInfo can be accepted.
An extra check for FADT ACPI table existence during installation step is
also added.
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
Notes:
v2:
- Function description updates [Sami]
- Refactorized the table verification [Pierre]
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
2 files changed, 103 insertions(+), 80 deletions(-)
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..4ad7c0c8dbfa 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -10,6 +10,7 @@
#include <Library/DebugLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/AcpiSystemDescriptionTable.h>
#include <Protocol/AcpiTable.h>
// Module specific include files.
@@ -22,6 +23,29 @@
#include <Protocol/DynamicTableFactoryProtocol.h>
#include <SmbiosTableGenerator.h>
+#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED BIT1
+
+#define ACPI_TABLE_VERIFY_FADT 0
+#define ACPI_TABLE_VERIFY_COUNT 6
+
+typedef struct {
+ ESTD_ACPI_TABLE_ID EstdTableId;
+ UINT32 AcpiTableSignature;
+ CHAR8 AcpiTableName[sizeof (UINT32) + 1];
+ BOOLEAN IsMandatory;
+ UINT16 Presence;
+} ACPI_TABLE_PRESENCE_INFO;
+
+ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
+ { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE, 0 },
+ { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", TRUE, 0 },
+ { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", TRUE, 0 },
+ { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, "DSDT", TRUE, 0 },
+ { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, "DBG2", FALSE, 0 },
+ { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR", FALSE, 0 },
+};
+
/** This macro expands to a function that retrieves the ACPI Table
List from the Configuration Manager.
*/
@@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
@retval EFI_SUCCESS Success.
@retval EFI_NOT_FOUND If mandatory table is not found.
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
**/
STATIC
EFI_STATUS
@@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
IN UINT32 AcpiTableCount
)
{
- EFI_STATUS Status;
- BOOLEAN FadtFound;
- BOOLEAN MadtFound;
- BOOLEAN GtdtFound;
- BOOLEAN DsdtFound;
- BOOLEAN Dbg2Found;
- BOOLEAN SpcrFound;
+ EFI_STATUS Status;
+ UINTN Handle;
+ UINTN Index;
+ UINTN InstalledTableIndex;
+ EFI_ACPI_DESCRIPTION_HEADER *DescHeader;
+ EFI_ACPI_TABLE_VERSION Version;
+ EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
- Status = EFI_SUCCESS;
- FadtFound = FALSE;
- MadtFound = FALSE;
- GtdtFound = FALSE;
- DsdtFound = FALSE;
- Dbg2Found = FALSE;
- SpcrFound = FALSE;
ASSERT (AcpiTableInfo != NULL);
+ Status = EFI_SUCCESS;
+
+ // Check against the statically initialized ACPI tables to see if they are in ACPI info list
while (AcpiTableCount-- != 0) {
- switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
- case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
- FadtFound = TRUE;
- break;
- case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
- MadtFound = TRUE;
- break;
- case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
- GtdtFound = TRUE;
- break;
- case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
- DsdtFound = TRUE;
- break;
- case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
- Dbg2Found = TRUE;
- break;
- case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
- SpcrFound = TRUE;
- break;
- default:
- break;
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
+ mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
+ }
}
}
- // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
- if (!FadtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
- Status = EFI_NOT_FOUND;
- }
+ // They also might be published already, so we can search from there
+ AcpiSdt = NULL;
+ Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
- if (!MadtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
- Status = EFI_NOT_FOUND;
+ if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
+ DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
+ goto EvaluatePresence;
}
- if (!GtdtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
- Status = EFI_NOT_FOUND;
- }
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ Handle = 0;
+ InstalledTableIndex = 0;
+ do {
+ Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
+ if (EFI_ERROR (Status)) {
+ break;
+ }
- if (!DsdtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
- Status = EFI_NOT_FOUND;
- }
+ InstalledTableIndex++;
+ } while (DescHeader->Signature != mAcpiVerifyTables[Index].AcpiTableSignature);
- if (!Dbg2Found) {
- DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
+ if (!EFI_ERROR (Status)) {
+ mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INSTALLED;
+ }
}
- if (!SpcrFound) {
- DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
+EvaluatePresence:
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ if (mAcpiVerifyTables[Index].Presence == 0) {
+ if (mAcpiVerifyTables[Index].IsMandatory) {
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ Status = EFI_NOT_FOUND;
+ } else {
+ DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ }
+ } else if (mAcpiVerifyTables[Index].Presence ==
+ (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLED))
+ {
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ Status = EFI_ALREADY_STARTED;
+ }
}
return Status;
@@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
@param [in] CfgMgrProtocol Pointer to the Configuration Manager
Protocol Interface.
- @retval EFI_SUCCESS Success.
- @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+ @retval EFI_SUCCESS Success.
+ @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
**/
STATIC
EFI_STATUS
@@ -562,7 +581,7 @@ ProcessAcpiTables (
if (EFI_ERROR (Status)) {
DEBUG ((
DEBUG_ERROR,
- "ERROR: Failed to find mandatory ACPI Table(s)."
+ "ERROR: Failed to verify mandatory ACPI Table(s) presence."
" Status = %r\n",
Status
));
@@ -570,29 +589,32 @@ ProcessAcpiTables (
}
// Add the FADT Table first.
- for (Idx = 0; Idx < AcpiTableCount; Idx++) {
- if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
- AcpiTableInfo[Idx].TableGeneratorId)
- {
- Status = BuildAndInstallAcpiTable (
- TableFactoryProtocol,
- CfgMgrProtocol,
- AcpiTableProtocol,
- &AcpiTableInfo[Idx]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((
- DEBUG_ERROR,
- "ERROR: Failed to find build and install ACPI FADT Table." \
- " Status = %r\n",
- Status
- ));
- return Status;
- }
+ if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRESENT_INSTALLED) == 0) {
+ // FADT is not yet installed
+ for (Idx = 0; Idx < AcpiTableCount; Idx++) {
+ if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
+ AcpiTableInfo[Idx].TableGeneratorId)
+ {
+ Status = BuildAndInstallAcpiTable (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ AcpiTableProtocol,
+ &AcpiTableInfo[Idx]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find build and install ACPI FADT Table." \
+ " Status = %r\n",
+ Status
+ ));
+ return Status;
+ }
- break;
- }
- } // for
+ break;
+ }
+ } // for
+ }
// Add remaining ACPI Tables
for (Idx = 0; Idx < AcpiTableCount; Idx++) {
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
index 028c3d413cf8..5ca98c8b4895 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -36,6 +36,7 @@ [LibraryClasses]
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+ gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
` (3 preceding siblings ...)
2022-07-28 4:31 ` [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
2022-07-28 4:31 ` [PATCH v2 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config Kun Qin
5 siblings, 1 reply; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Joe Lopez
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
Certain OSes will complain if the ECAM config space is not reserved in
the ACPI namespace.
This change adds a function to reserve PNP motherboard resources for a
given PCI node.
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
Notes:
v2:
- Only create RES0 after config space checking [Pierre]
DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 169 ++++++++++++++++++++
1 file changed, 169 insertions(+)
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..c03550baabf2 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,167 @@ GeneratePciCrs (
return Status;
}
+/** Generate a Pci Resource Template to hold Address Space Info
+
+ @param [in] PciNode RootNode of the AML tree.
+ @param [in, out] CrsNode CRS node of the AML tree to populate.
+
+ @retval EFI_SUCCESS The function completed successfully.
+ @retval EFI_INVALID_PARAMETER Invalid input parameter.
+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PopulateBasicPciResObjects (
+ IN AML_OBJECT_NODE_HANDLE PciNode,
+ IN OUT AML_OBJECT_NODE_HANDLE *CrsNode
+ )
+{
+ EFI_STATUS Status;
+ UINT32 EisaId;
+ AML_OBJECT_NODE_HANDLE ResNode;
+
+ if (CrsNode == NULL) {
+ ASSERT (0);
+ return EFI_INVALID_PARAMETER;
+ }
+
+ // ASL: Device (PCIx) {}
+ Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ // ASL: Name (_HID, EISAID ("PNP0C02"))
+ Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ // ASL: Name (_CRS, ResourceTemplate () {})
+ Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ return Status;
+}
+
+/** Generate a Pci Resource Template to hold Address Space Info
+
+ @param [in] Generator The SSDT Pci generator.
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager
+ Protocol interface.
+ @param [in] PciInfo Pci device information.
+ @param [in, out] PciNode RootNode of the AML tree to populate.
+
+ @retval EFI_SUCCESS The function completed successfully.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GeneratePciRes (
+ IN ACPI_PCI_GENERATOR *Generator,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
+ IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
+ IN OUT AML_OBJECT_NODE_HANDLE PciNode
+ )
+{
+ EFI_STATUS Status;
+ AML_OBJECT_NODE_HANDLE CrsNode;
+ BOOLEAN Translation;
+ UINT32 Index;
+ CM_ARM_OBJ_REF *RefInfo;
+ UINT32 RefCount;
+ CM_ARM_PCI_ADDRESS_MAP_INFO *AddrMapInfo;
+ BOOLEAN IsPosDecode;
+
+ // Get the array of CM_ARM_OBJ_REF referencing the
+ // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
+ Status = GetEArmObjCmRef (
+ CfgMgrProtocol,
+ PciInfo->AddressMapToken,
+ &RefInfo,
+ &RefCount
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ for (Index = 0; Index < RefCount; Index++) {
+ // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
+ Status = GetEArmObjPciAddressMapInfo (
+ CfgMgrProtocol,
+ RefInfo[Index].ReferenceToken,
+ &AddrMapInfo,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
+ if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
+ IsPosDecode = TRUE;
+ } else {
+ IsPosDecode = FALSE;
+ }
+
+ switch (AddrMapInfo->SpaceCode) {
+ case PCI_SS_CONFIG:
+ Status = PopulateBasicPciResObjects (PciNode, &CrsNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ break;
+ }
+
+ Status = AmlCodeGenRdQWordMemory (
+ FALSE,
+ IsPosDecode,
+ TRUE,
+ TRUE,
+ FALSE, // non-cacheable
+ TRUE,
+ 0,
+ AddrMapInfo->PciAddress,
+ AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
+ Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
+ AddrMapInfo->AddressSize,
+ 0,
+ NULL,
+ 0,
+ TRUE,
+ CrsNode,
+ NULL
+ );
+ break;
+ default:
+ break;
+ } // switch
+
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+ }
+
+ return Status;
+}
+
/** Generate a Pci device.
@param [in] Generator The SSDT Pci generator.
@@ -702,9 +863,17 @@ GeneratePciDevice (
return Status;
}
+ // Add the PNP Motherboard Resources Device to reserve ECAM space
+ Status = GeneratePciRes (Generator, CfgMgrProtocol, PciInfo, PciNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
// Add the template _OSC method.
Status = AddOscMethod (PciInfo, PciNode);
ASSERT_EFI_ERROR (Status);
+
return Status;
}
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
` (4 preceding siblings ...)
2022-07-28 4:31 ` [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space Kun Qin
@ 2022-07-28 4:31 ` Kun Qin
5 siblings, 0 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-28 4:31 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez, Pierre Gondois
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
This change added a switch case handling for PCI_SS_CONFIG during SSDT
generation. This will allow PCI config case return EFI_SUCCESS instead of
EFI_INVALID_PARAMETER.
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---
Notes:
v2:
- Added Reviewed-by tag [Pierre]
DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index c03550baabf2..2749cfabed6c 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -603,6 +603,11 @@ GeneratePciCrs (
);
break;
+ case PCI_SS_CONFIG:
+ // Do nothing
+ Status = EFI_SUCCESS;
+ break;
+
default:
Status = EFI_INVALID_PARAMETER;
} // switch
--
2.37.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
2022-07-28 4:31 ` [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables Kun Qin
@ 2022-07-28 13:07 ` PierreGondois
2022-07-29 5:00 ` Kun Qin
0 siblings, 1 reply; 12+ messages in thread
From: PierreGondois @ 2022-07-28 13:07 UTC (permalink / raw)
To: devel, kuqin12; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez
Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
Thanks!
On 7/28/22 06:31, Kun Qin via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>
> This change added an extra step to allow check for installed ACPI tables.
>
> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either pre-installed or
> supplied through AcpiTableInfo can be accepted.
>
> An extra check for FADT ACPI table existence during installation step is
> also added.
>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>
> Notes:
> v2:
> - Function description updates [Sami]
> - Refactorized the table verification [Pierre]
>
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
> 2 files changed, 103 insertions(+), 80 deletions(-)
>
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> index ed62299f9bbd..4ad7c0c8dbfa 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> @@ -10,6 +10,7 @@
> #include <Library/DebugLib.h>
> #include <Library/PcdLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/AcpiSystemDescriptionTable.h>
> #include <Protocol/AcpiTable.h>
>
> // Module specific include files.
> @@ -22,6 +23,29 @@
> #include <Protocol/DynamicTableFactoryProtocol.h>
> #include <SmbiosTableGenerator.h>
>
> +#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
> +#define ACPI_TABLE_PRESENT_INSTALLED BIT1
> +
> +#define ACPI_TABLE_VERIFY_FADT 0
> +#define ACPI_TABLE_VERIFY_COUNT 6
> +
> +typedef struct {
> + ESTD_ACPI_TABLE_ID EstdTableId;
> + UINT32 AcpiTableSignature;
> + CHAR8 AcpiTableName[sizeof (UINT32) + 1];
> + BOOLEAN IsMandatory;
> + UINT16 Presence;
> +} ACPI_TABLE_PRESENCE_INFO;
I think it needs some documentation (also for mAcpiVerifyTables).
> +
> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
> + { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE, 0 },
> + { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", TRUE, 0 },
> + { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", TRUE, 0 },
> + { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, "DSDT", TRUE, 0 },
> + { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, "DBG2", FALSE, 0 },
> + { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR", FALSE, 0 },
> +};
> +
> /** This macro expands to a function that retrieves the ACPI Table
> List from the Configuration Manager.
> */
> @@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
>
> @retval EFI_SUCCESS Success.
> @retval EFI_NOT_FOUND If mandatory table is not found.
> + @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
> **/
> STATIC
> EFI_STATUS
> @@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
> IN UINT32 AcpiTableCount
> )
> {
> - EFI_STATUS Status;
> - BOOLEAN FadtFound;
> - BOOLEAN MadtFound;
> - BOOLEAN GtdtFound;
> - BOOLEAN DsdtFound;
> - BOOLEAN Dbg2Found;
> - BOOLEAN SpcrFound;
> + EFI_STATUS Status;
> + UINTN Handle;
> + UINTN Index;
> + UINTN InstalledTableIndex;
> + EFI_ACPI_DESCRIPTION_HEADER *DescHeader;
> + EFI_ACPI_TABLE_VERSION Version;
> + EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
>
> - Status = EFI_SUCCESS;
> - FadtFound = FALSE;
> - MadtFound = FALSE;
> - GtdtFound = FALSE;
> - DsdtFound = FALSE;
> - Dbg2Found = FALSE;
> - SpcrFound = FALSE;
> ASSERT (AcpiTableInfo != NULL);
>
> + Status = EFI_SUCCESS;
> +
> + // Check against the statically initialized ACPI tables to see if they are in ACPI info list
> while (AcpiTableCount-- != 0) {
> - switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
> - case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
> - FadtFound = TRUE;
> - break;
> - case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
> - MadtFound = TRUE;
> - break;
> - case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
> - GtdtFound = TRUE;
> - break;
> - case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
> - DsdtFound = TRUE;
> - break;
> - case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
> - Dbg2Found = TRUE;
> - break;
> - case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
> - SpcrFound = TRUE;
> - break;
> - default:
> - break;
> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
> + if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
> + mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
Would it be possible to add a 'break' here ?
Just a note for Sami:
These double loops seem expensive, but I cannot find anything better and/or shorter.
> + }
> }
> }
>
> - // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
> - if (!FadtFound) {
> - DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
> - Status = EFI_NOT_FOUND;
> - }
> + // They also might be published already, so we can search from there
> + AcpiSdt = NULL;
> + Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
>
> - if (!MadtFound) {
> - DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
> - Status = EFI_NOT_FOUND;
> + if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
> + DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
> + goto EvaluatePresence;
I think this is ok to just print and return an error, unless you think about this could happen.
> }
>
> - if (!GtdtFound) {
> - DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
> - Status = EFI_NOT_FOUND;
> - }
> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
> + Handle = 0;
> + InstalledTableIndex = 0;
> + do {
> + Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
> + if (EFI_ERROR (Status)) {
> + break;
> + }
>
> - if (!DsdtFound) {
> - DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
> - Status = EFI_NOT_FOUND;
> - }
> + InstalledTableIndex++;
> + } while (DescHeader->Signature != mAcpiVerifyTables[Index].AcpiTableSignature);
>
> - if (!Dbg2Found) {
> - DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
> + if (!EFI_ERROR (Status)) {
> + mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INSTALLED;
> + }
> }
>
> - if (!SpcrFound) {
> - DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
> +EvaluatePresence:
> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
> + if (mAcpiVerifyTables[Index].Presence == 0) {
> + if (mAcpiVerifyTables[Index].IsMandatory) {
> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
> + Status = EFI_NOT_FOUND;
> + } else {
> + DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
> + }
> + } else if (mAcpiVerifyTables[Index].Presence ==
> + (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLED))
> + {
> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.\n", mAcpiVerifyTables[Index].AcpiTableName));
> + Status = EFI_ALREADY_STARTED;
> + }
> }
Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.
>
> return Status;
> @@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
> @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> Protocol Interface.
>
> - @retval EFI_SUCCESS Success.
> - @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
> + @retval EFI_SUCCESS Success.
> + @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
> + @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
> **/
> STATIC
> EFI_STATUS
> @@ -562,7 +581,7 @@ ProcessAcpiTables (
> if (EFI_ERROR (Status)) {
> DEBUG ((
> DEBUG_ERROR,
> - "ERROR: Failed to find mandatory ACPI Table(s)."
> + "ERROR: Failed to verify mandatory ACPI Table(s) presence."
> " Status = %r\n",
> Status
> ));
> @@ -570,29 +589,32 @@ ProcessAcpiTables (
> }
>
> // Add the FADT Table first.
> - for (Idx = 0; Idx < AcpiTableCount; Idx++) {
> - if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
> - AcpiTableInfo[Idx].TableGeneratorId)
> - {
> - Status = BuildAndInstallAcpiTable (
> - TableFactoryProtocol,
> - CfgMgrProtocol,
> - AcpiTableProtocol,
> - &AcpiTableInfo[Idx]
> - );
> - if (EFI_ERROR (Status)) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "ERROR: Failed to find build and install ACPI FADT Table." \
> - " Status = %r\n",
> - Status
> - ));
> - return Status;
> - }
> + if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRESENT_INSTALLED) == 0) {
> + // FADT is not yet installed
> + for (Idx = 0; Idx < AcpiTableCount; Idx++) {
> + if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
> + AcpiTableInfo[Idx].TableGeneratorId)
> + {
> + Status = BuildAndInstallAcpiTable (
> + TableFactoryProtocol,
> + CfgMgrProtocol,
> + AcpiTableProtocol,
> + &AcpiTableInfo[Idx]
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to find build and install ACPI FADT Table." \
> + " Status = %r\n",
> + Status
> + ));
> + return Status;
> + }
>
> - break;
> - }
> - } // for
> + break;
> + }
> + } // for
> + }
>
> // Add remaining ACPI Tables
> for (Idx = 0; Idx < AcpiTableCount; Idx++) {
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> index 028c3d413cf8..5ca98c8b4895 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> @@ -36,6 +36,7 @@ [LibraryClasses]
>
> [Protocols]
> gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> + gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED
>
> gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space
2022-07-28 4:31 ` [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space Kun Qin
@ 2022-07-28 13:07 ` PierreGondois
2022-07-29 5:01 ` Kun Qin
0 siblings, 1 reply; 12+ messages in thread
From: PierreGondois @ 2022-07-28 13:07 UTC (permalink / raw)
To: devel, kuqin12; +Cc: Joe Lopez, Sami Mujawar
Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
Thanks!
On 7/28/22 06:31, Kun Qin via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>
> Certain OSes will complain if the ECAM config space is not reserved in
> the ACPI namespace.
>
> This change adds a function to reserve PNP motherboard resources for a
> given PCI node.
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>
> Notes:
> v2:
> - Only create RES0 after config space checking [Pierre]
>
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 169 ++++++++++++++++++++
> 1 file changed, 169 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index ceffe2838c03..c03550baabf2 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -616,6 +616,167 @@ GeneratePciCrs (
> return Status;
> }
>
> +/** Generate a Pci Resource Template to hold Address Space Info
> +
> + @param [in] PciNode RootNode of the AML tree.
> + @param [in, out] CrsNode CRS node of the AML tree to populate.
I think CrsNode is an 'out' only parameter. Also the description of PciNode
seems incorrect.
> +
> + @retval EFI_SUCCESS The function completed successfully.
> + @retval EFI_INVALID_PARAMETER Invalid input parameter.
> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +PopulateBasicPciResObjects (
Would it be possible to rename it:
GenerateMotherboardDevice()
or with a name related to 'motherboard' ?
> + IN AML_OBJECT_NODE_HANDLE PciNode,
> + IN OUT AML_OBJECT_NODE_HANDLE *CrsNode
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 EisaId;
> + AML_OBJECT_NODE_HANDLE ResNode;
> +
> + if (CrsNode == NULL) {
> + ASSERT (0);
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // ASL: Device (PCIx) {}
> + Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + // ASL: Name (_HID, EISAID ("PNP0C02"))
> + Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + // ASL: Name (_CRS, ResourceTemplate () {})
> + Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + return Status;
> +}
> +
> +/** Generate a Pci Resource Template to hold Address Space Info
> +
> + @param [in] Generator The SSDT Pci generator.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol interface.
> + @param [in] PciInfo Pci device information.
> + @param [in, out] PciNode RootNode of the AML tree to populate.
> +
> + @retval EFI_SUCCESS The function completed successfully.
> + @retval EFI_INVALID_PARAMETER Invalid parameter.
> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GeneratePciRes (
Would it be possible to rename it:
ReserveEcamSpace()
or with a name related to 'ecam' ?
> + IN ACPI_PCI_GENERATOR *Generator,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
> + IN OUT AML_OBJECT_NODE_HANDLE PciNode
> + )
> +{
> + EFI_STATUS Status;
> + AML_OBJECT_NODE_HANDLE CrsNode;
> + BOOLEAN Translation;
> + UINT32 Index;
> + CM_ARM_OBJ_REF *RefInfo;
> + UINT32 RefCount;
> + CM_ARM_PCI_ADDRESS_MAP_INFO *AddrMapInfo;
> + BOOLEAN IsPosDecode;
> +
> + // Get the array of CM_ARM_OBJ_REF referencing the
> + // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
> + Status = GetEArmObjCmRef (
> + CfgMgrProtocol,
> + PciInfo->AddressMapToken,
> + &RefInfo,
> + &RefCount
> + );
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + for (Index = 0; Index < RefCount; Index++) {
> + // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
> + Status = GetEArmObjPciAddressMapInfo (
> + CfgMgrProtocol,
> + RefInfo[Index].ReferenceToken,
> + &AddrMapInfo,
> + NULL
> + );
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> + Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
> + if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
> + IsPosDecode = TRUE;
> + } else {
> + IsPosDecode = FALSE;
> + }
I think this should be done in:
case PCI_SS_CONFIG:
to only do it when necessary.
> +
> + switch (AddrMapInfo->SpaceCode) {
> + case PCI_SS_CONFIG:
> + Status = PopulateBasicPciResObjects (PciNode, &CrsNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + break;
> + }
> +
> + Status = AmlCodeGenRdQWordMemory (
> + FALSE,
> + IsPosDecode,
> + TRUE,
> + TRUE,
> + FALSE, // non-cacheable
> + TRUE,
> + 0,
> + AddrMapInfo->PciAddress,
> + AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> + Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
> + AddrMapInfo->AddressSize,
> + 0,
> + NULL,
> + 0,
> + TRUE,
> + CrsNode,
> + NULL
> + );
> + break;
> + default:
> + break;
> + } // switch
> +
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> + }
> +
> + return Status;
> +}
> +
> /** Generate a Pci device.
>
> @param [in] Generator The SSDT Pci generator.
> @@ -702,9 +863,17 @@ GeneratePciDevice (
> return Status;
> }
>
> + // Add the PNP Motherboard Resources Device to reserve ECAM space
> + Status = GeneratePciRes (Generator, CfgMgrProtocol, PciInfo, PciNode);
> + if (EFI_ERROR (Status)) {
> + ASSERT (0);
> + return Status;
> + }
> +
> // Add the template _OSC method.
> Status = AddOscMethod (PciInfo, PciNode);
> ASSERT_EFI_ERROR (Status);
> +
> return Status;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
@ 2022-07-29 5:00 ` Kun Qin
2022-07-29 15:11 ` PierreGondois
0 siblings, 1 reply; 12+ messages in thread
From: Kun Qin @ 2022-07-29 5:00 UTC (permalink / raw)
To: Pierre Gondois, devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez
Hi Pierre,
Thank you for your feedback. I will add more document/comments to the
newly define structure, as
well as the "break" as you suggested.
As per failure to locate "gEfiAcpiSdtProtocolGuid", my thought was that
this protocol could be enabled
per platform through
"gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol". So I did not
treat
the failure as a hard requirement (for the same reason, I did not add it
to the module Depex). Please let
me know whether you think this makes sense. Otherwise, I could replace
the "goto" logic with a check
for the same PCD and only conduct the routine if ACPI_SDT is expected.
Please also let me know if you have other suggestions.
Thanks,
Kun
On 7/28/2022 6:07 AM, Pierre Gondois wrote:
> Hello Kun,
> With the changes below:
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>
> Thanks!
>
> On 7/28/22 06:31, Kun Qin via groups.io wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>>
>> This change added an extra step to allow check for installed ACPI
>> tables.
>>
>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either
>> pre-installed or
>> supplied through AcpiTableInfo can be accepted.
>>
>> An extra check for FADT ACPI table existence during installation step is
>> also added.
>>
>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>
>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>
>> Notes:
>> v2:
>> - Function description updates [Sami]
>> - Refactorized the table verification [Pierre]
>>
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> | 182 +++++++++++---------
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> | 1 +
>> 2 files changed, 103 insertions(+), 80 deletions(-)
>>
>> diff --git
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>
>> index ed62299f9bbd..4ad7c0c8dbfa 100644
>> ---
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> +++
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> @@ -10,6 +10,7 @@
>> #include <Library/DebugLib.h>
>> #include <Library/PcdLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>> #include <Protocol/AcpiTable.h>
>> // Module specific include files.
>> @@ -22,6 +23,29 @@
>> #include <Protocol/DynamicTableFactoryProtocol.h>
>> #include <SmbiosTableGenerator.h>
>> +#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
>> +#define ACPI_TABLE_PRESENT_INSTALLED BIT1
>> +
>> +#define ACPI_TABLE_VERIFY_FADT 0
>> +#define ACPI_TABLE_VERIFY_COUNT 6
>> +
>> +typedef struct {
>> + ESTD_ACPI_TABLE_ID EstdTableId;
>> + UINT32 AcpiTableSignature;
>> + CHAR8 AcpiTableName[sizeof (UINT32) + 1];
>> + BOOLEAN IsMandatory;
>> + UINT16 Presence;
>> +} ACPI_TABLE_PRESENCE_INFO;
>
> I think it needs some documentation (also for mAcpiVerifyTables).
>
>> +
>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
>> + { EStdAcpiTableIdFadt,
>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE, 0 },
>> + { EStdAcpiTableIdMadt,
>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT",
>> TRUE, 0 },
>> + { EStdAcpiTableIdGtdt,
>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT",
>> TRUE, 0 },
>> + { EStdAcpiTableIdDsdt,
>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
>> "DSDT", TRUE, 0 },
>> + { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,
>> "DBG2", FALSE, 0 },
>> + { EStdAcpiTableIdSpcr,
>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR",
>> FALSE, 0 },
>> +};
>> +
>> /** This macro expands to a function that retrieves the ACPI Table
>> List from the Configuration Manager.
>> */
>> @@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
>> @retval EFI_SUCCESS Success.
>> @retval EFI_NOT_FOUND If mandatory table is not found.
>> + @retval EFI_ALREADY_STARTED If mandatory table found in
>> AcpiTableInfo is already installed.
>> **/
>> STATIC
>> EFI_STATUS
>> @@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
>> IN UINT32 AcpiTableCount
>> )
>> {
>> - EFI_STATUS Status;
>> - BOOLEAN FadtFound;
>> - BOOLEAN MadtFound;
>> - BOOLEAN GtdtFound;
>> - BOOLEAN DsdtFound;
>> - BOOLEAN Dbg2Found;
>> - BOOLEAN SpcrFound;
>> + EFI_STATUS Status;
>> + UINTN Handle;
>> + UINTN Index;
>> + UINTN InstalledTableIndex;
>> + EFI_ACPI_DESCRIPTION_HEADER *DescHeader;
>> + EFI_ACPI_TABLE_VERSION Version;
>> + EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
>> - Status = EFI_SUCCESS;
>> - FadtFound = FALSE;
>> - MadtFound = FALSE;
>> - GtdtFound = FALSE;
>> - DsdtFound = FALSE;
>> - Dbg2Found = FALSE;
>> - SpcrFound = FALSE;
>> ASSERT (AcpiTableInfo != NULL);
>> + Status = EFI_SUCCESS;
>> +
>> + // Check against the statically initialized ACPI tables to see if
>> they are in ACPI info list
>> while (AcpiTableCount-- != 0) {
>> - switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>> - case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>> - FadtFound = TRUE;
>> - break;
>> - case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>> - MadtFound = TRUE;
>> - break;
>> - case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>> - GtdtFound = TRUE;
>> - break;
>> - case
>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>> - DsdtFound = TRUE;
>> - break;
>> - case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>> - Dbg2Found = TRUE;
>> - break;
>> - case
>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>> - SpcrFound = TRUE;
>> - break;
>> - default:
>> - break;
>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>> + if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature ==
>> mAcpiVerifyTables[Index].AcpiTableSignature) {
>> + mAcpiVerifyTables[Index].Presence |=
>> ACPI_TABLE_PRESENT_INFO_LIST;
>
> Would it be possible to add a 'break' here ?
>
> Just a note for Sami:
> These double loops seem expensive, but I cannot find anything better
> and/or shorter.
>
>> + }
>> }
>> }
>> - // We need at least the FADT, MADT, GTDT and the DSDT tables to
>> boot
>> - if (!FadtFound) {
>> - DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>> - Status = EFI_NOT_FOUND;
>> - }
>> + // They also might be published already, so we can search from there
>> + AcpiSdt = NULL;
>> + Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL,
>> (VOID **)&AcpiSdt);
>> - if (!MadtFound) {
>> - DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>> - Status = EFI_NOT_FOUND;
>> + if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>> + DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol
>> (0x%p) - %r\n", AcpiSdt, Status));
>> + goto EvaluatePresence;
>
> I think this is ok to just print and return an error, unless you think
> about this could happen.
>
>
>> }
>> - if (!GtdtFound) {
>> - DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>> - Status = EFI_NOT_FOUND;
>> - }
>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>> + Handle = 0;
>> + InstalledTableIndex = 0;
>> + do {
>> + Status = AcpiSdt->GetAcpiTable (InstalledTableIndex,
>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>> + if (EFI_ERROR (Status)) {
>> + break;
>> + }
>> - if (!DsdtFound) {
>> - DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>> - Status = EFI_NOT_FOUND;
>> - }
>> + InstalledTableIndex++;
>> + } while (DescHeader->Signature !=
>> mAcpiVerifyTables[Index].AcpiTableSignature);
>> - if (!Dbg2Found) {
>> - DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>> + if (!EFI_ERROR (Status)) {
>> + mAcpiVerifyTables[Index].Presence |=
>> ACPI_TABLE_PRESENT_INSTALLED;
>> + }
>> }
>> - if (!SpcrFound) {
>> - DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>> +EvaluatePresence:
>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>> + if (mAcpiVerifyTables[Index].Presence == 0) {
>> + if (mAcpiVerifyTables[Index].IsMandatory) {
>> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n",
>> mAcpiVerifyTables[Index].AcpiTableName));
>> + Status = EFI_NOT_FOUND;
>> + } else {
>> + DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n",
>> mAcpiVerifyTables[Index].AcpiTableName));
>> + }
>> + } else if (mAcpiVerifyTables[Index].Presence ==
>> + (ACPI_TABLE_PRESENT_INFO_LIST |
>> ACPI_TABLE_PRESENT_INSTALLED))
>> + {
>> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already
>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>> + Status = EFI_ALREADY_STARTED;
>> + }
>> }
> Just a note for Sami:
> In the loop above we return the last invalid code, this should be ok.
>
>
>> return Status;
>> @@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
>> @param [in] CfgMgrProtocol Pointer to the Configuration
>> Manager
>> Protocol Interface.
>> - @retval EFI_SUCCESS Success.
>> - @retval EFI_NOT_FOUND If a mandatory table or a generator is not
>> found.
>> + @retval EFI_SUCCESS Success.
>> + @retval EFI_NOT_FOUND If a mandatory table or a generator
>> is not found.
>> + @retval EFI_ALREADY_STARTED If mandatory table found in
>> AcpiTableInfo is already installed.
>> **/
>> STATIC
>> EFI_STATUS
>> @@ -562,7 +581,7 @@ ProcessAcpiTables (
>> if (EFI_ERROR (Status)) {
>> DEBUG ((
>> DEBUG_ERROR,
>> - "ERROR: Failed to find mandatory ACPI Table(s)."
>> + "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>> " Status = %r\n",
>> Status
>> ));
>> @@ -570,29 +589,32 @@ ProcessAcpiTables (
>> }
>> // Add the FADT Table first.
>> - for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>> - if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>> - AcpiTableInfo[Idx].TableGeneratorId)
>> - {
>> - Status = BuildAndInstallAcpiTable (
>> - TableFactoryProtocol,
>> - CfgMgrProtocol,
>> - AcpiTableProtocol,
>> - &AcpiTableInfo[Idx]
>> - );
>> - if (EFI_ERROR (Status)) {
>> - DEBUG ((
>> - DEBUG_ERROR,
>> - "ERROR: Failed to find build and install ACPI FADT Table." \
>> - " Status = %r\n",
>> - Status
>> - ));
>> - return Status;
>> - }
>> + if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence &
>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
>> + // FADT is not yet installed
>> + for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>> + if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>> + AcpiTableInfo[Idx].TableGeneratorId)
>> + {
>> + Status = BuildAndInstallAcpiTable (
>> + TableFactoryProtocol,
>> + CfgMgrProtocol,
>> + AcpiTableProtocol,
>> + &AcpiTableInfo[Idx]
>> + );
>> + if (EFI_ERROR (Status)) {
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "ERROR: Failed to find build and install ACPI FADT
>> Table." \
>> + " Status = %r\n",
>> + Status
>> + ));
>> + return Status;
>> + }
>> - break;
>> - }
>> - } // for
>> + break;
>> + }
>> + } // for
>> + }
>> // Add remaining ACPI Tables
>> for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>> diff --git
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>
>> index 028c3d413cf8..5ca98c8b4895 100644
>> ---
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> +++
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> @@ -36,6 +36,7 @@ [LibraryClasses]
>> [Protocols]
>> gEfiAcpiTableProtocolGuid # PROTOCOL
>> ALWAYS_CONSUMED
>> + gEfiAcpiSdtProtocolGuid # PROTOCOL
>> ALWAYS_CONSUMED
>> gEdkiiConfigurationManagerProtocolGuid # PROTOCOL
>> ALWAYS_CONSUMED
>> gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL
>> ALWAYS_CONSUMED
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
@ 2022-07-29 5:01 ` Kun Qin
0 siblings, 0 replies; 12+ messages in thread
From: Kun Qin @ 2022-07-29 5:01 UTC (permalink / raw)
To: Pierre Gondois, devel; +Cc: Joe Lopez, Sami Mujawar
Hi Pierre,
Thank you for the suggestions. I will update the patch per your feedback and
send for review in v3.
Regards,
Kun
On 7/28/2022 6:07 AM, Pierre Gondois wrote:
> Hello Kun,
> With the changes below:
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>
> Thanks!
>
>
> On 7/28/22 06:31, Kun Qin via groups.io wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>>
>> Certain OSes will complain if the ECAM config space is not reserved in
>> the ACPI namespace.
>>
>> This change adds a function to reserve PNP motherboard resources for a
>> given PCI node.
>>
>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>
>> Notes:
>> v2:
>> - Only create RES0 after config space checking [Pierre]
>>
>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> | 169 ++++++++++++++++++++
>> 1 file changed, 169 insertions(+)
>>
>> diff --git
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>
>> index ceffe2838c03..c03550baabf2 100644
>> ---
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> +++
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> @@ -616,6 +616,167 @@ GeneratePciCrs (
>> return Status;
>> }
>> +/** Generate a Pci Resource Template to hold Address Space Info
>> +
>> + @param [in] PciNode RootNode of the AML tree.
>> + @param [in, out] CrsNode CRS node of the AML tree to populate.
>
> I think CrsNode is an 'out' only parameter. Also the description of
> PciNode
> seems incorrect.
>
>> +
>> + @retval EFI_SUCCESS The function completed successfully.
>> + @retval EFI_INVALID_PARAMETER Invalid input parameter.
>> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PopulateBasicPciResObjects (
>
> Would it be possible to rename it:
> GenerateMotherboardDevice()
> or with a name related to 'motherboard' ?
>
>> + IN AML_OBJECT_NODE_HANDLE PciNode,
>> + IN OUT AML_OBJECT_NODE_HANDLE *CrsNode
>> + )
>> +{
>> + EFI_STATUS Status;
>> + UINT32 EisaId;
>> + AML_OBJECT_NODE_HANDLE ResNode;
>> +
>> + if (CrsNode == NULL) {
>> + ASSERT (0);
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + // ASL: Device (PCIx) {}
>> + Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + // ASL: Name (_HID, EISAID ("PNP0C02"))
>> + Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP
>> Motherboard Resources */
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + // ASL: Name (_CRS, ResourceTemplate () {})
>> + Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + return Status;
>> +}
>> +
>> +/** Generate a Pci Resource Template to hold Address Space Info
>> +
>> + @param [in] Generator The SSDT Pci generator.
>> + @param [in] CfgMgrProtocol Pointer to the Configuration
>> Manager
>> + Protocol interface.
>> + @param [in] PciInfo Pci device information.
>> + @param [in, out] PciNode RootNode of the AML tree to
>> populate.
>> +
>> + @retval EFI_SUCCESS The function completed successfully.
>> + @retval EFI_INVALID_PARAMETER Invalid parameter.
>> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GeneratePciRes (
>
> Would it be possible to rename it:
> ReserveEcamSpace()
> or with a name related to 'ecam' ?
>
>
>> + IN ACPI_PCI_GENERATOR *Generator,
>> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
>> CfgMgrProtocol,
>> + IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
>> + IN OUT AML_OBJECT_NODE_HANDLE PciNode
>> + )
>> +{
>> + EFI_STATUS Status;
>> + AML_OBJECT_NODE_HANDLE CrsNode;
>> + BOOLEAN Translation;
>> + UINT32 Index;
>> + CM_ARM_OBJ_REF *RefInfo;
>> + UINT32 RefCount;
>> + CM_ARM_PCI_ADDRESS_MAP_INFO *AddrMapInfo;
>> + BOOLEAN IsPosDecode;
>> +
>> + // Get the array of CM_ARM_OBJ_REF referencing the
>> + // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>> + Status = GetEArmObjCmRef (
>> + CfgMgrProtocol,
>> + PciInfo->AddressMapToken,
>> + &RefInfo,
>> + &RefCount
>> + );
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + for (Index = 0; Index < RefCount; Index++) {
>> + // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>> + Status = GetEArmObjPciAddressMapInfo (
>> + CfgMgrProtocol,
>> + RefInfo[Index].ReferenceToken,
>> + &AddrMapInfo,
>> + NULL
>> + );
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> + Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
>> + if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>> + IsPosDecode = TRUE;
>> + } else {
>> + IsPosDecode = FALSE;
>> + }
>
> I think this should be done in:
> case PCI_SS_CONFIG:
> to only do it when necessary.
>
>> +
>> + switch (AddrMapInfo->SpaceCode) {
>> + case PCI_SS_CONFIG:
>> + Status = PopulateBasicPciResObjects (PciNode, &CrsNode);
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + break;
>> + }
>> +
>> + Status = AmlCodeGenRdQWordMemory (
>> + FALSE,
>> + IsPosDecode,
>> + TRUE,
>> + TRUE,
>> + FALSE, // non-cacheable
>> + TRUE,
>> + 0,
>> + AddrMapInfo->PciAddress,
>> + AddrMapInfo->PciAddress +
>> AddrMapInfo->AddressSize - 1,
>> + Translation ? AddrMapInfo->CpuAddress -
>> AddrMapInfo->PciAddress : 0,
>> + AddrMapInfo->AddressSize,
>> + 0,
>> + NULL,
>> + 0,
>> + TRUE,
>> + CrsNode,
>> + NULL
>> + );
>> + break;
>> + default:
>> + break;
>> + } // switch
>> +
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> + }
>> +
>> + return Status;
>> +}
>> +
>> /** Generate a Pci device.
>> @param [in] Generator The SSDT Pci generator.
>> @@ -702,9 +863,17 @@ GeneratePciDevice (
>> return Status;
>> }
>> + // Add the PNP Motherboard Resources Device to reserve ECAM space
>> + Status = GeneratePciRes (Generator, CfgMgrProtocol, PciInfo,
>> PciNode);
>> + if (EFI_ERROR (Status)) {
>> + ASSERT (0);
>> + return Status;
>> + }
>> +
>> // Add the template _OSC method.
>> Status = AddOscMethod (PciInfo, PciNode);
>> ASSERT_EFI_ERROR (Status);
>> +
>> return Status;
>> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
2022-07-29 5:00 ` Kun Qin
@ 2022-07-29 15:11 ` PierreGondois
0 siblings, 0 replies; 12+ messages in thread
From: PierreGondois @ 2022-07-29 15:11 UTC (permalink / raw)
To: Kun Qin, devel; +Cc: Sami Mujawar, Alexei Fedorov, Joe Lopez
On 7/29/22 07:00, Kun Qin wrote:
> Hi Pierre,
>
> Thank you for your feedback. I will add more document/comments to the
> newly define structure, as
> well as the "break" as you suggested.
>
> As per failure to locate "gEfiAcpiSdtProtocolGuid", my thought was that
> this protocol could be enabled
> per platform through
> "gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol". So I did not
> treat
> the failure as a hard requirement (for the same reason, I did not add it
> to the module Depex). Please let
> me know whether you think this makes sense. Otherwise, I could replace
> the "goto" logic with a check
> for the same PCD and only conduct the routine if ACPI_SDT is expected.
>
Ok yes, this is a good idea,
Regards,
Pierre
> Please also let me know if you have other suggestions.
>
> Thanks,
> Kun
>
> On 7/28/2022 6:07 AM, Pierre Gondois wrote:
>> Hello Kun,
>> With the changes below:
>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>
>> Thanks!
>>
>> On 7/28/22 06:31, Kun Qin via groups.io wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>>>
>>> This change added an extra step to allow check for installed ACPI
>>> tables.
>>>
>>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either
>>> pre-installed or
>>> supplied through AcpiTableInfo can be accepted.
>>>
>>> An extra check for FADT ACPI table existence during installation step is
>>> also added.
>>>
>>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - Function description updates [Sami]
>>> - Refactorized the table verification [Pierre]
>>>
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> | 182 +++++++++++---------
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> | 1 +
>>> 2 files changed, 103 insertions(+), 80 deletions(-)
>>>
>>> diff --git
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>>
>>> index ed62299f9bbd..4ad7c0c8dbfa 100644
>>> ---
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> +++
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> @@ -10,6 +10,7 @@
>>> #include <Library/DebugLib.h>
>>> #include <Library/PcdLib.h>
>>> #include <Library/UefiBootServicesTableLib.h>
>>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>>> #include <Protocol/AcpiTable.h>
>>> // Module specific include files.
>>> @@ -22,6 +23,29 @@
>>> #include <Protocol/DynamicTableFactoryProtocol.h>
>>> #include <SmbiosTableGenerator.h>
>>> +#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
>>> +#define ACPI_TABLE_PRESENT_INSTALLED BIT1
>>> +
>>> +#define ACPI_TABLE_VERIFY_FADT 0
>>> +#define ACPI_TABLE_VERIFY_COUNT 6
>>> +
>>> +typedef struct {
>>> + ESTD_ACPI_TABLE_ID EstdTableId;
>>> + UINT32 AcpiTableSignature;
>>> + CHAR8 AcpiTableName[sizeof (UINT32) + 1];
>>> + BOOLEAN IsMandatory;
>>> + UINT16 Presence;
>>> +} ACPI_TABLE_PRESENCE_INFO;
>>
>> I think it needs some documentation (also for mAcpiVerifyTables).
>>
>>> +
>>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
>>> + { EStdAcpiTableIdFadt,
>>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE, 0 },
>>> + { EStdAcpiTableIdMadt,
>>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT",
>>> TRUE, 0 },
>>> + { EStdAcpiTableIdGtdt,
>>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT",
>>> TRUE, 0 },
>>> + { EStdAcpiTableIdDsdt,
>>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
>>> "DSDT", TRUE, 0 },
>>> + { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,
>>> "DBG2", FALSE, 0 },
>>> + { EStdAcpiTableIdSpcr,
>>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR",
>>> FALSE, 0 },
>>> +};
>>> +
>>> /** This macro expands to a function that retrieves the ACPI Table
>>> List from the Configuration Manager.
>>> */
>>> @@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
>>> @retval EFI_SUCCESS Success.
>>> @retval EFI_NOT_FOUND If mandatory table is not found.
>>> + @retval EFI_ALREADY_STARTED If mandatory table found in
>>> AcpiTableInfo is already installed.
>>> **/
>>> STATIC
>>> EFI_STATUS
>>> @@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
>>> IN UINT32 AcpiTableCount
>>> )
>>> {
>>> - EFI_STATUS Status;
>>> - BOOLEAN FadtFound;
>>> - BOOLEAN MadtFound;
>>> - BOOLEAN GtdtFound;
>>> - BOOLEAN DsdtFound;
>>> - BOOLEAN Dbg2Found;
>>> - BOOLEAN SpcrFound;
>>> + EFI_STATUS Status;
>>> + UINTN Handle;
>>> + UINTN Index;
>>> + UINTN InstalledTableIndex;
>>> + EFI_ACPI_DESCRIPTION_HEADER *DescHeader;
>>> + EFI_ACPI_TABLE_VERSION Version;
>>> + EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
>>> - Status = EFI_SUCCESS;
>>> - FadtFound = FALSE;
>>> - MadtFound = FALSE;
>>> - GtdtFound = FALSE;
>>> - DsdtFound = FALSE;
>>> - Dbg2Found = FALSE;
>>> - SpcrFound = FALSE;
>>> ASSERT (AcpiTableInfo != NULL);
>>> + Status = EFI_SUCCESS;
>>> +
>>> + // Check against the statically initialized ACPI tables to see if
>>> they are in ACPI info list
>>> while (AcpiTableCount-- != 0) {
>>> - switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>>> - case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>>> - FadtFound = TRUE;
>>> - break;
>>> - case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>>> - MadtFound = TRUE;
>>> - break;
>>> - case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>>> - GtdtFound = TRUE;
>>> - break;
>>> - case
>>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>>> - DsdtFound = TRUE;
>>> - break;
>>> - case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>>> - Dbg2Found = TRUE;
>>> - break;
>>> - case
>>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>>> - SpcrFound = TRUE;
>>> - break;
>>> - default:
>>> - break;
>>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>> + if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature ==
>>> mAcpiVerifyTables[Index].AcpiTableSignature) {
>>> + mAcpiVerifyTables[Index].Presence |=
>>> ACPI_TABLE_PRESENT_INFO_LIST;
>>
>> Would it be possible to add a 'break' here ?
>>
>> Just a note for Sami:
>> These double loops seem expensive, but I cannot find anything better
>> and/or shorter.
>>
>>> + }
>>> }
>>> }
>>> - // We need at least the FADT, MADT, GTDT and the DSDT tables to
>>> boot
>>> - if (!FadtFound) {
>>> - DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>>> - Status = EFI_NOT_FOUND;
>>> - }
>>> + // They also might be published already, so we can search from there
>>> + AcpiSdt = NULL;
>>> + Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL,
>>> (VOID **)&AcpiSdt);
>>> - if (!MadtFound) {
>>> - DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>>> - Status = EFI_NOT_FOUND;
>>> + if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>>> + DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol
>>> (0x%p) - %r\n", AcpiSdt, Status));
>>> + goto EvaluatePresence;
>>
>> I think this is ok to just print and return an error, unless you think
>> about this could happen.
>>
>>
>>> }
>>> - if (!GtdtFound) {
>>> - DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>>> - Status = EFI_NOT_FOUND;
>>> - }
>>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>> + Handle = 0;
>>> + InstalledTableIndex = 0;
>>> + do {
>>> + Status = AcpiSdt->GetAcpiTable (InstalledTableIndex,
>>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>>> + if (EFI_ERROR (Status)) {
>>> + break;
>>> + }
>>> - if (!DsdtFound) {
>>> - DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>>> - Status = EFI_NOT_FOUND;
>>> - }
>>> + InstalledTableIndex++;
>>> + } while (DescHeader->Signature !=
>>> mAcpiVerifyTables[Index].AcpiTableSignature);
>>> - if (!Dbg2Found) {
>>> - DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>>> + if (!EFI_ERROR (Status)) {
>>> + mAcpiVerifyTables[Index].Presence |=
>>> ACPI_TABLE_PRESENT_INSTALLED;
>>> + }
>>> }
>>> - if (!SpcrFound) {
>>> - DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>>> +EvaluatePresence:
>>> + for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>> + if (mAcpiVerifyTables[Index].Presence == 0) {
>>> + if (mAcpiVerifyTables[Index].IsMandatory) {
>>> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n",
>>> mAcpiVerifyTables[Index].AcpiTableName));
>>> + Status = EFI_NOT_FOUND;
>>> + } else {
>>> + DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n",
>>> mAcpiVerifyTables[Index].AcpiTableName));
>>> + }
>>> + } else if (mAcpiVerifyTables[Index].Presence ==
>>> + (ACPI_TABLE_PRESENT_INFO_LIST |
>>> ACPI_TABLE_PRESENT_INSTALLED))
>>> + {
>>> + DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already
>>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>>> + Status = EFI_ALREADY_STARTED;
>>> + }
>>> }
>> Just a note for Sami:
>> In the loop above we return the last invalid code, this should be ok.
>>
>>
>>> return Status;
>>> @@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
>>> @param [in] CfgMgrProtocol Pointer to the Configuration
>>> Manager
>>> Protocol Interface.
>>> - @retval EFI_SUCCESS Success.
>>> - @retval EFI_NOT_FOUND If a mandatory table or a generator is not
>>> found.
>>> + @retval EFI_SUCCESS Success.
>>> + @retval EFI_NOT_FOUND If a mandatory table or a generator
>>> is not found.
>>> + @retval EFI_ALREADY_STARTED If mandatory table found in
>>> AcpiTableInfo is already installed.
>>> **/
>>> STATIC
>>> EFI_STATUS
>>> @@ -562,7 +581,7 @@ ProcessAcpiTables (
>>> if (EFI_ERROR (Status)) {
>>> DEBUG ((
>>> DEBUG_ERROR,
>>> - "ERROR: Failed to find mandatory ACPI Table(s)."
>>> + "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>>> " Status = %r\n",
>>> Status
>>> ));
>>> @@ -570,29 +589,32 @@ ProcessAcpiTables (
>>> }
>>> // Add the FADT Table first.
>>> - for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>> - if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>> - AcpiTableInfo[Idx].TableGeneratorId)
>>> - {
>>> - Status = BuildAndInstallAcpiTable (
>>> - TableFactoryProtocol,
>>> - CfgMgrProtocol,
>>> - AcpiTableProtocol,
>>> - &AcpiTableInfo[Idx]
>>> - );
>>> - if (EFI_ERROR (Status)) {
>>> - DEBUG ((
>>> - DEBUG_ERROR,
>>> - "ERROR: Failed to find build and install ACPI FADT Table." \
>>> - " Status = %r\n",
>>> - Status
>>> - ));
>>> - return Status;
>>> - }
>>> + if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence &
>>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
>>> + // FADT is not yet installed
>>> + for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>> + if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>> + AcpiTableInfo[Idx].TableGeneratorId)
>>> + {
>>> + Status = BuildAndInstallAcpiTable (
>>> + TableFactoryProtocol,
>>> + CfgMgrProtocol,
>>> + AcpiTableProtocol,
>>> + &AcpiTableInfo[Idx]
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + DEBUG ((
>>> + DEBUG_ERROR,
>>> + "ERROR: Failed to find build and install ACPI FADT
>>> Table." \
>>> + " Status = %r\n",
>>> + Status
>>> + ));
>>> + return Status;
>>> + }
>>> - break;
>>> - }
>>> - } // for
>>> + break;
>>> + }
>>> + } // for
>>> + }
>>> // Add remaining ACPI Tables
>>> for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>> diff --git
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>>
>>> index 028c3d413cf8..5ca98c8b4895 100644
>>> ---
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> +++
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> @@ -36,6 +36,7 @@ [LibraryClasses]
>>> [Protocols]
>>> gEfiAcpiTableProtocolGuid # PROTOCOL
>>> ALWAYS_CONSUMED
>>> + gEfiAcpiSdtProtocolGuid # PROTOCOL
>>> ALWAYS_CONSUMED
>>> gEdkiiConfigurationManagerProtocolGuid # PROTOCOL
>>> ALWAYS_CONSUMED
>>> gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL
>>> ALWAYS_CONSUMED
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-07-29 15:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-28 4:31 [PATCH v2 0/6] Enhance DynamicTablesPkg modules Kun Qin
2022-07-28 4:31 ` [PATCH v2 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf Kun Qin
2022-07-28 4:31 ` [PATCH v2 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing Kun Qin
2022-07-28 4:31 ` [PATCH v2 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers Kun Qin
2022-07-28 4:31 ` [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables Kun Qin
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
2022-07-29 5:00 ` Kun Qin
2022-07-29 15:11 ` PierreGondois
2022-07-28 4:31 ` [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space Kun Qin
2022-07-28 13:07 ` [edk2-devel] " PierreGondois
2022-07-29 5:01 ` Kun Qin
2022-07-28 4:31 ` [PATCH v2 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config Kun Qin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox