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@arm.com>

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

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;

  }