[edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

2022-07-27 Thread Kun Qin
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 
Signed-off-by: Kun Qin 
---

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_RESOURCESCould not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PopulateBasicPciResObjects (
+  INAML_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_RESOURCESCould not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GeneratePciRes (
+  INACPI_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_HANDLEPciNode
+  )
+{
+  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,
+

Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

2022-07-28 Thread PierreGondois

Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois 

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 
Signed-off-by: Kun Qin 
---

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_RESOURCESCould not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PopulateBasicPciResObjects (


Would it be possible to rename it:
   GenerateMotherboardDevice()
or with a name related to 'motherboard' ?


+  INAML_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_RESOURCESCould not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GeneratePciRes (


Would it be possible to rename it:
   ReserveEcamSpace()
or with a name related to 'ecam' ?



+  INACPI_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_HANDLEPciNode
+  )
+{
+  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

Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

2022-07-28 Thread Kun Qin

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 

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 
Signed-off-by: Kun Qin 
---

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 = (Add