[PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space


Kun Qin
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3998

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@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <pierre.gondois@...>
---

Notes:
v2:
- Only create RES0 after config space checking [Pierre]
=20=20=20=20
v3:
- Updated function names and descriptions [Pierre]
- Moved translation calculation to CONFIG case [Pierre]

DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c |=
171 ++++++++++++++++++++
1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieG=
enerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieG=
enerator.c
index ceffe2838c03..658a089c8f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerato=
r.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerato=
r.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
return Status;=0D
}=0D
=0D
+/** Generate a RES0 device node to reserve PNP motherboard resources=0D
+ for a given PCI node.=0D
+=0D
+ @param [in] PciNode Parent PCI node handle of the generated=0D
+ resource object.=0D
+ @param [out] CrsNode CRS node of the AML tree to populate.=0D
+=0D
+ @retval EFI_SUCCESS The function completed successfully.=0D
+ @retval EFI_INVALID_PARAMETER Invalid input parameter.=0D
+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory.=0D
+**/=0D
+STATIC=0D
+EFI_STATUS=0D
+EFIAPI=0D
+GenerateMotherboardDevice (=0D
+ IN AML_OBJECT_NODE_HANDLE PciNode,=0D
+ OUT AML_OBJECT_NODE_HANDLE *CrsNode=0D
+ )=0D
+{=0D
+ EFI_STATUS Status;=0D
+ UINT32 EisaId;=0D
+ AML_OBJECT_NODE_HANDLE ResNode;=0D
+=0D
+ if (CrsNode =3D=3D NULL) {=0D
+ ASSERT (0);=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+=0D
+ // ASL: Device (RES0) {}=0D
+ Status =3D AmlCodeGenDevice ("RES0", PciNode, &ResNode);=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ // ASL: Name (_HID, EISAID ("PNP0C02"))=0D
+ Status =3D AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboa=
rd Resources */=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ Status =3D AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ // ASL: Name (_CRS, ResourceTemplate () {})=0D
+ Status =3D AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ return Status;=0D
+}=0D
+=0D
+/** Reserves ECAM space for PCI config space=0D
+=0D
+ @param [in] Generator The SSDT Pci generator.=0D
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager=0D
+ Protocol interface.=0D
+ @param [in] PciInfo Pci device information.=0D
+ @param [in, out] PciNode RootNode of the AML tree to populate.=
=0D
+=0D
+ @retval EFI_SUCCESS The function completed successfully.=0D
+ @retval EFI_INVALID_PARAMETER Invalid parameter.=0D
+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory.=0D
+**/=0D
+STATIC=0D
+EFI_STATUS=0D
+EFIAPI=0D
+ReserveEcamSpace (=0D
+ IN ACPI_PCI_GENERATOR *Generator,=
=0D
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtoc=
ol,=0D
+ IN CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,=0D
+ IN OUT AML_OBJECT_NODE_HANDLE PciNode=0D
+ )=0D
+{=0D
+ EFI_STATUS Status;=0D
+ AML_OBJECT_NODE_HANDLE CrsNode;=0D
+ BOOLEAN Translation;=0D
+ UINT32 Index;=0D
+ CM_ARM_OBJ_REF *RefInfo;=0D
+ UINT32 RefCount;=0D
+ CM_ARM_PCI_ADDRESS_MAP_INFO *AddrMapInfo;=0D
+ BOOLEAN IsPosDecode;=0D
+=0D
+ // Get the array of CM_ARM_OBJ_REF referencing the=0D
+ // CM_ARM_PCI_ADDRESS_MAP_INFO objects.=0D
+ Status =3D GetEArmObjCmRef (=0D
+ CfgMgrProtocol,=0D
+ PciInfo->AddressMapToken,=0D
+ &RefInfo,=0D
+ &RefCount=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ for (Index =3D 0; Index < RefCount; Index++) {=0D
+ // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.=0D
+ Status =3D GetEArmObjPciAddressMapInfo (=0D
+ CfgMgrProtocol,=0D
+ RefInfo[Index].ReferenceToken,=0D
+ &AddrMapInfo,=0D
+ NULL=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
+ switch (AddrMapInfo->SpaceCode) {=0D
+ case PCI_SS_CONFIG:=0D
+ Translation =3D (AddrMapInfo->CpuAddress !=3D AddrMapInfo->PciAddr=
ess);=0D
+ if (AddrMapInfo->CpuAddress >=3D AddrMapInfo->PciAddress) {=0D
+ IsPosDecode =3D TRUE;=0D
+ } else {=0D
+ IsPosDecode =3D FALSE;=0D
+ }=0D
+=0D
+ Status =3D GenerateMotherboardDevice (PciNode, &CrsNode);=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ break;=0D
+ }=0D
+=0D
+ Status =3D AmlCodeGenRdQWordMemory (=0D
+ FALSE,=0D
+ IsPosDecode,=0D
+ TRUE,=0D
+ TRUE,=0D
+ FALSE, // non-cacheable=0D
+ TRUE,=0D
+ 0,=0D
+ AddrMapInfo->PciAddress,=0D
+ AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,=
=0D
+ Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->Pc=
iAddress : 0,=0D
+ AddrMapInfo->AddressSize,=0D
+ 0,=0D
+ NULL,=0D
+ 0,=0D
+ TRUE,=0D
+ CrsNode,=0D
+ NULL=0D
+ );=0D
+ break;=0D
+ default:=0D
+ break;=0D
+ } // switch=0D
+=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+ }=0D
+=0D
+ return Status;=0D
+}=0D
+=0D
/** Generate a Pci device.=0D
=0D
@param [in] Generator The SSDT Pci generator.=0D
@@ -702,9 +865,17 @@ GeneratePciDevice (
return Status;=0D
}=0D
=0D
+ // Add the PNP Motherboard Resources Device to reserve ECAM space=0D
+ Status =3D ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, PciNode=
);=0D
+ if (EFI_ERROR (Status)) {=0D
+ ASSERT (0);=0D
+ return Status;=0D
+ }=0D
+=0D
// Add the template _OSC method.=0D
Status =3D AddOscMethod (PciInfo, PciNode);=0D
ASSERT_EFI_ERROR (Status);=0D
+=0D
return Status;=0D
}=0D
=0D
--=20
2.37.1.windows.1


Sami Mujawar
 

Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, 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@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <pierre.gondois@...>
---

Notes:
v2:
- Only create RES0 after config space checking [Pierre]
v3:
- Updated function names and descriptions [Pierre]
- Moved translation calculation to CONFIG case [Pierre]

DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..658a089c8f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
return Status;

}


+/** Generate a RES0 device node to reserve PNP motherboard resources

+ for a given PCI node.

+

+ @param [in] PciNode Parent PCI node handle of the generated

+ resource object.

+ @param [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

+GenerateMotherboardDevice (

+ IN AML_OBJECT_NODE_HANDLE PciNode,

+ 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 (RES0) {}

+ 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;

+}

+

+/** Reserves ECAM space for PCI config space

+

+ @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

+ReserveEcamSpace (

+ 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;

+ }

+

+ switch (AddrMapInfo->SpaceCode) {

+ case PCI_SS_CONFIG:

+ Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);

+ if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {

+ IsPosDecode = TRUE;

+ } else {

+ IsPosDecode = FALSE;

+ }

+

+ Status = GenerateMotherboardDevice (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 +865,17 @@ GeneratePciDevice (
return Status;

}


+ // Add the PNP Motherboard Resources Device to reserve ECAM space

+ Status = ReserveEcamSpace (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;

}


Sami Mujawar
 

Hi Kun,

I have just tried testing your patch with Kvmtool guest firmware and think this patch may need some modifications.

Also, the patch 4/6 may need some adjustment, which I will reply back on that patch separately.

Regards,

Sami Mujawar

On 08/08/2022 02:22 pm, Sami Mujawar wrote:
Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, 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@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <pierre.gondois@...>
---

Notes:
     v2:
     - Only create RES0 after config space checking [Pierre]
          v3:
     - Updated function names and descriptions [Pierre]
     - Moved translation calculation to CONFIG case [Pierre]

  DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
  1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..658a089c8f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
    return Status;

  }

 
+/** Generate a RES0 device node to reserve PNP motherboard resources

+  for a given PCI node.

+

+  @param [in]   PciNode       Parent PCI node handle of the generated

+                              resource object.

+  @param [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

+GenerateMotherboardDevice (

+  IN  AML_OBJECT_NODE_HANDLE  PciNode,

+  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 (RES0) {}

+  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;

+}

+

+/** Reserves ECAM space for PCI config space

+

+  @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

+ReserveEcamSpace (

+  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;

+    }

[SAMI] Sorry for missing this earlier in the review. However, the ECAM memory space is described by CM_ARM_PCI_CONFIG_SPACE_INFO. So, I think that needs to be used here.

The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length of the configuration space and would probably need to be updated.

Which platform are you testing these changes on? I would like to understand more about your use case. Is it possible to share some more details, please?

[/SAMI]

+

+    switch (AddrMapInfo->SpaceCode) {

+      case PCI_SS_CONFIG:

+        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);

+        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {

+          IsPosDecode = TRUE;

+        } else {

+          IsPosDecode = FALSE;

+        }

+

+        Status = GenerateMotherboardDevice (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 +865,17 @@ GeneratePciDevice (
      return Status;

    }

 
+  // Add the PNP Motherboard Resources Device to reserve ECAM space

+  Status = ReserveEcamSpace (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;

  }

 


PierreGondois
 

On 8/8/22 17:39, Sami Mujawar wrote:
Hi Kun,
I have just tried testing your patch with Kvmtool guest firmware and think this patch may need some modifications.
Also, the patch 4/6 may need some adjustment, which I will reply back on that patch separately.
Regards,
Sami Mujawar
On 08/08/2022 02:22 pm, Sami Mujawar wrote:
Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, 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@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <pierre.gondois@...>
---

Notes:
     v2:
     - Only create RES0 after config space checking [Pierre]
          v3:
     - Updated function names and descriptions [Pierre]
     - Moved translation calculation to CONFIG case [Pierre]

DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
  1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..658a089c8f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
    return Status;

  }


+/** Generate a RES0 device node to reserve PNP motherboard resources

+  for a given PCI node.

+

+  @param [in]   PciNode       Parent PCI node handle of the generated

+                              resource object.

+  @param [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

+GenerateMotherboardDevice (

+  IN  AML_OBJECT_NODE_HANDLE  PciNode,

+  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 (RES0) {}

+  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;

+}

+

+/** Reserves ECAM space for PCI config space

+

+  @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

+ReserveEcamSpace (

+  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;

+    }
[SAMI] Sorry for missing this earlier in the review. However, the ECAM memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I think that needs to be used here.
The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length of the configuration space and would probably need to be updated.
Which platform are you testing these changes on? I would like to understand more about your use case. Is it possible to share some more details, please?
[/SAMI]
[Pierre]

Yes indeed, CM_ARM_PCI_CONFIG_SPACE_INFO should contain the configuration
address space, it should not be described in CM_ARM_PCI_ADDRESS_MAP_INFO.
So there should be no need to search through the CM_ARM_PCI_ADDRESS_MAP_INFO
objects. Sorry for missing this earlier.

The length of the address space could be computed as:
length = (end_bus - start_bus + 1) × 32 devices × 8 functions × 4 KB

[/Pierre]


+

+    switch (AddrMapInfo->SpaceCode) {

+      case PCI_SS_CONFIG:

+        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);

+        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {

+          IsPosDecode = TRUE;

+        } else {

+          IsPosDecode = FALSE;

+        }

+

+        Status = GenerateMotherboardDevice (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 +865,17 @@ GeneratePciDevice (
      return Status;

    }


+  // Add the PNP Motherboard Resources Device to reserve ECAM space

+  Status = ReserveEcamSpace (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;

  }


Kun Qin
 

Hi Pierre/Sami,

Thanks for your feedback. We modified the routine to be based on
`CM_ARM_PCI_CONFIG_SPACE_INFO` and sanity checked the table
outputs from UEFI shell and verified the system bootabilty on VDK
based ARM platform.

The new patch can be found here:
https://edk2.groups.io/g/devel/message/92317

Looking forward to your further feedback.

Thanks,
Kun

On 8/10/2022 1:51 AM, Pierre Gondois wrote:


On 8/8/22 17:39, Sami Mujawar wrote:
Hi Kun,

I have just tried testing your patch with Kvmtool guest firmware and think this patch may need some modifications.

Also, the patch 4/6 may need some adjustment, which I will reply back on that patch separately.

Regards,

Sami Mujawar

On 08/08/2022 02:22 pm, Sami Mujawar wrote:
Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, 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@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <pierre.gondois@...>
---

Notes:
     v2:
     - Only create RES0 after config space checking [Pierre]
          v3:
     - Updated function names and descriptions [Pierre]
     - Moved translation calculation to CONFIG case [Pierre]

DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
  1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..658a089c8f1f 100644
---
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
    return Status;

  }


+/** Generate a RES0 device node to reserve PNP motherboard resources

+  for a given PCI node.

+

+  @param [in]   PciNode       Parent PCI node handle of the generated

+                              resource object.

+  @param [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

+GenerateMotherboardDevice (

+  IN  AML_OBJECT_NODE_HANDLE  PciNode,

+  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 (RES0) {}

+  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;

+}

+

+/** Reserves ECAM space for PCI config space

+

+  @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

+ReserveEcamSpace (

+  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;

+    }
[SAMI] Sorry for missing this earlier in the review. However, the ECAM memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I think that needs to be used here.

The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length of the configuration space and would probably need to be updated.

Which platform are you testing these changes on? I would like to understand more about your use case. Is it possible to share some more details, please?

[/SAMI]
[Pierre]

Yes indeed, CM_ARM_PCI_CONFIG_SPACE_INFO should contain the configuration
address space, it should not be described in CM_ARM_PCI_ADDRESS_MAP_INFO.
So there should be no need to search through the CM_ARM_PCI_ADDRESS_MAP_INFO
objects. Sorry for missing this earlier.

The length of the address space could be computed as:
length = (end_bus - start_bus + 1) × 32 devices × 8 functions × 4 KB

[/Pierre]


+

+    switch (AddrMapInfo->SpaceCode) {

+      case PCI_SS_CONFIG:

+        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);

+        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {

+          IsPosDecode = TRUE;

+        } else {

+          IsPosDecode = FALSE;

+        }

+

+        Status = GenerateMotherboardDevice (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 +865,17 @@ GeneratePciDevice (
      return Status;

    }


+  // Add the PNP Motherboard Resources Device to reserve ECAM space

+  Status = ReserveEcamSpace (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;

  }